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

Bump Python requirements in setup.cfg and rmm_dev.yml #982

Merged
merged 8 commits into from
Mar 21, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Feb 15, 2022

No description provided.

@shwina shwina requested a review from a team as a code owner February 15, 2022 19:08
@github-actions github-actions bot added the Python Related to RMM Python API label Feb 15, 2022
@shwina shwina added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 15, 2022
@leofang
Copy link
Member

leofang commented Feb 15, 2022

While you're at it, I think it'd be nice to also update this file? conda/environments/rmm_dev_cuda11.5.yml

@bdice
Copy link
Contributor

bdice commented Feb 15, 2022

Good catch @leofang. Other places referencing Python 3.7:

  • conda/environments/*.yml
  • README.md
  • setup.py classifiers
  • Several places in ci/local (which probably doesn't work at present due to other problems like outdated CUDA images -- maybe just leave this code alone)

@shwina shwina requested a review from a team as a code owner February 15, 2022 19:37
@github-actions github-actions bot added the conda label Feb 15, 2022
@shwina shwina changed the title Bump Python requirements in setup.cfg Bump Python requirements in setup.cfg and rmm_dev.yml Feb 15, 2022
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
python/setup.cfg Show resolved Hide resolved
conda/environments/rmm_dev_cuda11.5.yml Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Ashwin! 😀

Had a couple questions below

README.md Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@ dependencies:
- flake8=3.8.3
- black=19.10
- isort=5.6.4
- python>=3.7,<3.9
- python>=3.8,<3.10
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the upper bound here? Just wondering since we are dropping elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes -- these are the .yml files used to create dev environments. Since we don't typically want development with 3.10, we keep the upper bound here. This doesn't stop developers from manually building RMM with Python 3.10 if they desire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to do something like the following?

conda create -f rmm_dev_env.yml python=3.9

If so, we can (in theory) drop the upper bound here and make it the developer's responsibility to specify which Python they want to use.

Copy link
Contributor

@bdice bdice Feb 16, 2022

Choose a reason for hiding this comment

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

That’s a good idea — I have seen other packages specify a yml with a user-provided python spec (edit: apparently this is not a thing, I was wrong). Then update the README/docs to include that specification in the commands to create a conda environment.

On that note, it might even make sense to remove the upper bound (use >=11.5) for cudatoolkit and recommend that users pick their version as needed, instead of maintaining two environment files for 11.5 and 11.6.

Copy link
Contributor Author

@shwina shwina Feb 16, 2022

Choose a reason for hiding this comment

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

But is that a valid conda command, and does it do the expected thing here (override the python spec in the .yml?)

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Darn. @shwina I could have sworn I had seen this before but the specifiers after -f stuff.yml are ignored, so you can't do what I was suggesting. Sorry about that. 😢

Co-authored-by: jakirkham <jakirkham@gmail.com>
@shwina
Copy link
Contributor Author

shwina commented Feb 16, 2022

rerun tests

@shwina shwina added this to PR-WIP in v22.04 Release via automation Feb 22, 2022
@github-actions
Copy link

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@vyasr
Copy link
Contributor

vyasr commented Mar 21, 2022

@shwina what's holding this PR up? Is it just waiting on a resolution of the upper bound question above?

@shwina
Copy link
Contributor Author

shwina commented Mar 21, 2022

@vyasr I think that discussion can be resolved. This should be good to merge front m my perspective.

@vyasr
Copy link
Contributor

vyasr commented Mar 21, 2022

@bdice @jakirkham let me know if either of you thinks that discussion is a blocker, otherwise I'll aim to merge this by EOD to get ahead of burndown. Trying to avoid surprises ahead of code freeze mid-GTC.

@jakirkham
Copy link
Member

Nothing blocking from me

@vyasr
Copy link
Contributor

vyasr commented Mar 21, 2022

Received approval from @bdice offline. Merging now

@vyasr
Copy link
Contributor

vyasr commented Mar 21, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 851d3ba into rapidsai:branch-22.04 Mar 21, 2022
v22.04 Release automation moved this from PR-WIP to Done Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conda improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants