-
Notifications
You must be signed in to change notification settings - Fork 155
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
Add a pyproject.toml file. #516
Conversation
I think the workflow would work as is, if you approve it, but for the sake of brevity it might be worth modifying the actions file to only python -m pip install .[test] and remove the line with logic that installs requirements.txt, if it exists (requirements are now in pyproject.toml) |
Thanks for the PR @Justin-Hoffman. Are you able to see the workflow logs? If so would you mind having a look and fixing? |
I can see and will take a look. |
52877a4
to
77aeee7
Compare
This passes a local run of xvfb-run -a pytest, so I expect this workflow likely will as well (barring any unexpected version related issues). At risk of polluting the commit I also made nam/_version.py a generated file. The version number is now generated dynamically from existing tags. Locally my builds end up as version 0.11.1.dev4, because the last tag in the repo is 0.11.0, so the patch is incremented and dev4 is appended because this commit isn't a tag and we're 4 commits past the most recent tag (4 commits past 0.11.0) |
77aeee7
to
dfe3dcc
Compare
Remove setup.py
dfe3dcc
to
d9070b1
Compare
Bumping this @sdatkinson - Let me know if there's anything else you'd like to see here. |
Sorry for the delay. I've been busy this week. Going to find time in the coming week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
Addresses #487
Remove setup.py and move to pyproject.toml.
Right now this results in duplicate instances of project version ( inside pyproject.toml and inside ./nam/_version.py)I'm sure there's an elegant way to avoid this, I just haven't done it yet. I wanted to open the MR for feedback as is.
^ This is addressed in the second commit.
With this change you can still just
python3 -m pip install .
Both the workaround for the older transformer/numpy library and test dependencies are expressed as optional dependencies and can be installed with:
python3 -m pip install .[test] (or .[transformer-compat])
But maybe it's time to remove that workaround?
I haven't addressed the conda environments files, but I propose that we put:
- neural-amp-modeler
or possibly:
- neural-amp-modeler[test]
under the dependencies/pip section and remove the requirements.txt (which is redundant because they are in pyproject.toml now)
The environment files can likely get a de-duplication pass as well.