Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Porting to pyproject.toml #6

Merged
merged 33 commits into from
Nov 21, 2022
Merged

Porting to pyproject.toml #6

merged 33 commits into from
Nov 21, 2022

Conversation

JoeZiminski
Copy link
Member

@JoeZiminski JoeZiminski commented Nov 10, 2022

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.
  1. project urls seem a straightforward move, but I dont think spaces are not supported. Is this lowercase format an issue?

  2. [Zip safe depreciated ](Add zip_safe support python-poetry/poetry#928 zip_safe obsolete)so removed

  3. the two things I am quite unsure of,

  • I moved setuptools_scm into requires and updated the version based on here and here . This builds okay with pip, but I am not sure what is going on under the hood and when it may fail
  • Similarly, the find command, I copied from setup.cfg using new sytax based on this

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

@adamltyson
Copy link
Member

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
Copy link
Member Author

JoeZiminski commented Nov 11, 2022

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
Copy link
Member

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
Copy link
Member

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

@adamltyson
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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:

  • Use bump2version to create a git tag with a meaningful semantic version
  • setuptools_scm will then read this tag and use it to "report" the version when needed.

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

@niksirbi
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@adamltyson
Copy link
Member

@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).

@adamltyson
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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

@niksirbi
Copy link
Member

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.

* [black](https://black.readthedocs.io/en/stable/) for code structure formatting (maximum line length set to 79)
* [flake8](https://flake8.pycqa.org/en/latest/) to enforce [PEP8](https://www.python.org/dev/peps/pep-0008/)
* [mypy](https://mypy.readthedocs.io/en/stable/index.html) a static type checker
* [isort](https://pycqa.github.io/isort/) sorts imports alphabetically
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's an alphabetical sort, it is much smarter than that

```

**Automated versioning**
## Automated versioning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version should be now re-written for setuptools_scm, perhaps Joe can help

@adamltyson adamltyson mentioned this pull request Nov 18, 2022
@adamltyson
Copy link
Member

adamltyson commented Nov 21, 2022

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

@lauraporta
Copy link
Member

OK I make a separate branch for the docs

@niksirbi
Copy link
Member

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.

@niksirbi niksirbi merged commit 45ad4b5 into main Nov 21, 2022
@niksirbi niksirbi deleted the porting_to_pyproject.toml branch December 9, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants