neuroinformatics-unit / python-cookiecutter

Utility to create a basic Python project structure with tests, CI etc.
BSD 3-Clause "New" or "Revised" License
24 stars 3 forks source link

Porting to pyproject.toml #6

Closed JoeZiminski closed 2 years ago

JoeZiminski commented 2 years ago

PR to port setup.cfg functionality from setup.cfg to pyproject.toml due to planned depreciation of setup.cfg.

For the most part the corresponding arangement is clear, but not for all, and for some things I am not certain what the command are actually doing so would be good to get feedback. Currently the whole process

cookiecutter https://github.com/SainsburyWellcomeCentre/python-cookiecutter --checkout porting_to_pyproject.toml

then pip install -e .[dev] install without issue (windows 10, in pycharm, conda env).

1) Name, version, author, author email, url, license, description, long_rescription, classifiers straightforward move

2) long_description_content_type I think is depreciated, all I could find on it was from here:

“[Different structures for the readme field](https://peps.python.org/pep-0621/#different-structures-for-the-readme-field)
The readme field had a proposed readme_content_type field, but the authors considered the string/table hybrid more practical for the common case while still accommodating the more complex case. Same goes for using long_description and a corresponding long_description_content_type field.

3) project urls seem a straightforward move, but I dont think spaces are not supported. Is this lowercase format an issue?

4) [Zip safe depreciated ](https://github.com/python-poetry/poetry/issues/928 zip_safe obsolete)so removed

5) the two things I am quite unsure of,

Overall it runs okay for me, but not sure if there are other use cases we need to test.

adamltyson commented 2 years ago

Should we add tests for this? How do we add tests for this? It would be good to have a test that "pretends" to be a user, goes through the options and checks the end result is installable, and that bump2version etc work.

Is there a clever way to do something like this?

JoeZiminski commented 2 years ago

It would be nice to have tests, it seems you can pass a config file (https://cookiecutter.readthedocs.io/en/1.7.2/advanced/user_config.html?highlight=config) to avoid the CLI input interface. Would have to use subprocess() a lot but I think this is okay:

subprocess cookiecutter XXX + use config file check the folder structure is as expected check the config fig is as expected, parinsg the toml https://pypi.org/project/toml/ use subprocess pip install... and subprocess pip show... and check the stdout?

[note: testing pre-commit testing bump2version?]

lauraporta commented 2 years ago

I am trying to install the repo at this branch and I am having issues in ubuntu.

I run pip install -e .[dev] in my conda environment and I receive this error: ERROR: file:///home/lauraporta/Source/github/SainsburyWellcomeCenter/python-cookiecutter does not appear to be a Python project: neither 'setup.py' nor 'pyproject.toml' found.

Was the .toml file deleted by mistake or it should be as it is?

lauraporta commented 2 years ago

Issue solved thanks to IRL Joe assistance 🤣️ I misunderstood how to install the current cookiecutter.

adamltyson commented 2 years ago

Based on this page it seems that some fields have changed name when moving from setup.cfg to pyproject.toml, e.g. [options.extras_require] -> [project.optional-dependencies]. Probably worth checking all of these to see what's changed (and another reason to add tests).

niksirbi commented 2 years ago

@JoeZiminski good job, I think you made the right calls. Question: Shouldn't the below lines be also included in pyproject.toml to activate setuptools_scm? Unless I misunderstood the instructions. Or is it impled that setuptools_scm will run with default config without any additional lines in pyproject.toml?

# pyproject.toml
[tool.setuptools_scm]
niksirbi commented 2 years ago

I tested my comment above. I pip installed setuptools_scm into my environment for debugging purposes (for this reason might be a good idea to also add it to the dev requirements, not only to the build requirements). I ran python -m setuptools_scm in my project folder, and got the following:

Warning: could not use pyproject.toml, using default configuration.
 Reason: /home/niko/Code/wazp/pyproject.toml does not contain a tool.setuptools_scm section.
/home/niko/anaconda3/envs/zoo/lib/python3.10/site-packages/setuptools_scm/config.py:61: UserWarning: relative_to is expected to be a file, its the directory '.'
assuming the parent directory was passed
  warnings.warn(
('ERROR: no version found for', Namespace(root=None, config=None, strip_dev=False, command=None))
niksirbi commented 2 years ago

I now fixed the setuptools_scm issue I described above. Additionally, I changed the bump2version config file. It was still looking for version in setup.cfg (which no longer exists), so I changed it to pyproject.toml.

Outstanding issue: I think currently bump2version and setuptools_scm are not configured in the way suggested here

adamltyson commented 2 years ago

Additionally, I changed the bump2version config file. It was still looking for version in setup.cfg (which no longer exists), so I changed it to pyproject.toml.

My understanding is that to use both tools, bump2version will no longer set the version anywhere. I thought the workflow would be:

I haven't used setuptools_scm though, so this could be incorrect.

niksirbi commented 2 years ago

What I don't currently understand is whether the current .bump2version.cfg already creates the git tag, or whether we need to add sth like this:

# Instructs the tool to automatically commit the changes in Git
# with the give message and tag information
# based on the old and new version numbers.
#
commit = True
sign_tags = False
tag_message = Release v{new_version}
message = Bump v{current_version} -> v{new_version}
lauraporta commented 2 years ago

I saw from the last commits we are not supporting python 3.7 in pyproject.toml classifiers, but test_and_deployment.yml still indicates python 3.7 in the tests. Indeed, tests fail in my PR in the repo load-suite2p. I am going to edit test_and_deployment.yml

lauraporta commented 2 years ago

Tests now pass ✅ but deploy is skipped ⌽ I am not sure this is the expected behaviour.

adamltyson commented 2 years ago

@lauraporta that's intended. deploy should only run on a tagged commit. The idea is to use bump2version to change the version number, and create a tagged commit. When this is pushed, it triggers deployment (as long as the tests pass).

JoeZiminski commented 2 years ago

The package isn't installing properly to site-packages, but is written to build directory in the repo (if using pip install . from directory). Also, the build folder was empty. pushed commit to ensure build file is populated.

JoeZiminski commented 2 years ago

Okay the package now installs pip install . with what I think is the expected behaviour.

.
└── package-name/
    ├── files
    ├── module_name/
    │   └── packages and files
    └── tests

In this case everything in 'module_name' is installed at site-packages/module_name and a .dist-info file is also written.

For the build and egg file that are created and not cleaned up from package-name, this seems like expected behaviour with pip and need to be manually deleted

niksirbi commented 2 years ago

@adamltyson I created a project with the current version of cookiecutter, I ran pre-commit install, and upon my next commit I was asked to log into gitlab (which was not happening before). From the terminal output, I assume it has to do with flake8:

INFO] Initializing environment for https://github.com/pycqa/isort.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://gitlab.com/pycqa/flake8.
Enter basic credentials for 'https://gitlab.com/':
adamltyson commented 2 years ago

@niksirbi Turns out flake8 moved from gitlab to github some time ago. The gitlab repo was live until sometime yesterday, so now I think a lot of pre-commits are going to start failing.

Anyway, I've updated the pre-commit file here, and I suppose I'll start fixing a lot of repos 👎

JoeZiminski commented 2 years ago

@niksirbi Turns out flake8 moved from gitlab to github some time ago. The gitlab repo was live until sometime yesterday, so now I think a lot of pre-commits are going to start failing.

Anyway, I've updated the pre-commit file here, and I suppose I'll start fixing a lot of repos 👎

Thanks for this, have been banging my head against the wall as to why pre-commit is trying to authenticate against the flake8 repo

lauraporta commented 2 years ago

I added an edit to the MANIFEST.in file and .flake8 following error messages that I had in the checks done within a PR. They can now pass without failing.

adamltyson commented 2 years ago

@lauraporta, these things have been added to the manifest, but do we actually want them in the built package? They aren't used when installed. I think they can be excluded, and check-manifest will be equally happy.

include *.yaml
include .bumpversion.cfg
include tox.ini
recursive-include tests *.py
include .flake8
lauraporta commented 2 years ago

I am expanding the README.md file in this branch since it will include details relative on the changes present here.

@adamltyson, I asked check-manifest and it seems not happy, but maybe I am using it wrongly. I run it in without parameters, this is the error message I get:

> check-manifest
lists of files in version control and sdist do not match!
missing from sdist:
  .bumpversion.cfg
  .flake8
  .pre-commit-config.yaml
  tests/__init__.py
  tests/test_integration/__init__.py
  tests/test_unit/__init__.py
  tests/test_unit/test_placeholder.py
  tox.ini
suggested MANIFEST.in rules:
  include *.yaml
  include .bumpversion.cfg
  include tox.ini
  recursive-include tests *.py
adamltyson commented 2 years ago

The manifest lists what files should and shouldn't end up in the built package (python files and some others end up there by default). check-manifest compares what's in the built dist to the repo and lets you know if they don't match.

It's suggesting files are added to the manifest, but we don't need those files in the built package, so I think check-manifest will be equally happy if they are explicitly excluded, rather than included.

JoeZiminski commented 2 years ago

setuptools_scm is working now. Its a bit of a pain as we discussed, to have to lookup the most recent version from git-tags, and manaully imput the version number, and remember to git push --tags rather than git push --all which could catch out newcomers.

However, it is nice to not have to worry about anything hard-coded in the project, and it avoids any worries of the bump2version and setuptools_scm diverging.

JoeZiminski commented 2 years ago

all CI is passing, but I see set-output and 12 node.js activations get a depreciation warning, do we need to worry about these?

adamltyson commented 2 years ago

all CI is passing, but I see set-output and 12 node.js activations get a depreciation warning, do we need to worry about these?

I think those warnings are from the actions we use, and it's up to those authors to (hopefully) update things.

niksirbi commented 2 years ago

@adamltyson I encountered the same set-output conflict in my website actions, and I know how to fix it. I will do it now for this repo as well because this syntax will break in 2023.

niksirbi commented 2 years ago

@JoeZiminski re versioning. I think we just need to document really well and explicitly the git push --tags thing

niksirbi commented 2 years ago

So the set-output warning is produced by actions/setup-python@v3. They have fixed it in v4 though, so we should consider switching our actions to that unless it's too big of a hassle. But I guess this is more an issue for the neuroinformatics-unit/actions repository, rather than this one.

adamltyson commented 2 years ago

Is this ready to review? As previously suggested, we should work on the docs in a seperate branch/PR.

lauraporta commented 2 years ago

OK I make a separate branch for the docs

niksirbi commented 2 years ago

I have taken the bold step to resolve the merge conflict and finally merge this giant PR. We'll resolve the remaining documentation issues in other branches.