-
Notifications
You must be signed in to change notification settings - Fork 15
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
Replacing Setup.py with Pyproject.py #279
Conversation
…d recursive packaging from test-requirements.txt and dev-requirements.txt, removed setup* from tox.ini,
…alidate pyproject.toml
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.
Tests seem to be failing.
…ted and 3.8 support will end this year
@@ -9,7 +9,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python: [ '3.7', '3.8', '3.9', '3.10' ] |
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.
3.7 is no longer being supported and support for 3.8 is ending this year: https://devguide.python.org/versions/
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.
Can you no longer install on python 3.7 /8 now? Or is it just not being tested.
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.
Just out of an abundance of caution we might want to check what version of python the terra and all of us images are using.
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.
Terra/AOU uses python3.10.12
cromshell on p3.7 is no longer being tested
I had some trouble installing dev packages on py3.11 because mypy==0.770 was not supported.
So I updated this to the latest version, the latest version doesn't work on 3.7. None of the packages in the requirements have been updated so cromshell should still work on py3.7 but if anyone is developing on 3.7 they'll run into package issues.
cython | ||
sphinx | ||
sphinx-rtd-theme | ||
mypy==0.770 |
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.
needed to updated because latest version of Py3.11 did not support this version
tox | ||
wheel | ||
bumpversion | ||
twine | ||
black | ||
flake8 | ||
pylint | ||
tomli==2.0.1 |
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.
needed for tox validation of pyproject.toml
cython | ||
sphinx | ||
sphinx-rtd-theme | ||
mypy==0.770 | ||
-r requirements.txt |
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.
pyproject.toml does not support recursive package installation
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.
How do transitive dependencies get installed then?
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.
I mean to say pyproject.toml requires a dependency txt file to list packages directly, and not point to another dependency file.
Transitive dependencies are still handled by pip during installation. The requiremnts.txt file is listed as core dependencies in the pyprojec.toml, and pip will install the transitive dependencies. The dev-requirements and test-requirements are listed as optional dependencies
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.
Thank you for the clarification!
pylint --exit-zero src | ||
|
||
[gh-actions] |
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.
The original thought was to have this be used during github testing, but the github actions list the python version to test so its is not needed here
@@ -80,9 +80,6 @@ jobs: | |||
- name: Setup Env | |||
run: python3 -m pip install --upgrade pip build | |||
|
|||
- name: Validate setup.py |
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.
There is no integrated solution for validating the pyproject.toml file. There are some third-party application but they are unstable.
I've noticed though that tox run excutting its test automatically does a basic level check of the toml file using not a cli command but via package called "tomli". This has been added to the dev-requirments.txt file
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.
@bshifaw Looks fine to me, but I definitely don't understand the nuance here.
I think we should probably check with the people using it for the all of us workspaces to make sure they have 3.9 available.
cython | ||
sphinx | ||
sphinx-rtd-theme | ||
mypy==0.770 | ||
-r requirements.txt |
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.
How do transitive dependencies get installed then?
pyproject.toml
Outdated
@@ -1,3 +1,5 @@ | |||
|
|||
ignore_missing_imports = true |
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.
Can you explain this one to me? Don't we need all our imports?
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.
Thanks for this catch, its suppose to be under [tool.mypy."pytest.*"]
When ignore_missing_imports is set to true, mypy will not report an error when an imported module cannot be found. This is to replicate the setting that was originally found in the setup.cfg file
@@ -9,7 +9,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python: [ '3.7', '3.8', '3.9', '3.10' ] |
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.
Can you no longer install on python 3.7 /8 now? Or is it just not being tested.
@@ -9,7 +9,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python: [ '3.7', '3.8', '3.9', '3.10' ] |
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.
Just out of an abundance of caution we might want to check what version of python the terra and all of us images are using.
This update was to help fix the installation on AOU workspaces. When running their cromshell tutorial, the installation included a cell that updated the cromshell version by pip - git (pip install git+https://github.com/broadinstitute/cromshell). This caused the installation error mentioned in the PR description. Their python version is 3.10.3. Also, to confirm I just now installed cromshell in an AOU notebook using this specific branch; the instillation now completes successfully. |
@@ -49,9 +53,9 @@ deps = | |||
flake8 | |||
|
|||
commands = | |||
black --check --diff --target-version py38 src tests setup.py | |||
isort --check-only --profile black --diff -rc tests src setup.py |
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.
-rc
for isort is deprecated because options now happen by default
The python community is moving towards pyproject as the standard method of specifying a project metadata.
The Cromshell repo has been using a mix of pyproject.py and setup.py, this is currently causing issue during installation.
Changes in this repo replaceses setup.py completely with pyproject.toml, fixing the installation errors.