Skip to content

Migrate to pip only and drop pipenv #6265

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.

Sign up for GitHub

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

Jump to bottom
Merged
sfc-gh-kbregula merged 2 commits into from
Apr 6, 2023
Merged

Migrate to pip only and drop pipenv #6265

sfc-gh-kbregula merged 2 commits into from
Apr 6, 2023

Conversation

@sfc-gh-kbregula
Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula commented Mar 8, 2023 •
edited
Loading

📚 Context

Continuation of work:
#6234
I previously dealt with generating constraints files on our CI. This is blocked for now as we wait for the security team to confirm that we can keep the files in the repository.

I have already started working further and see that these files do not work properly with the latest version of pip because there are dependency conflicts, as we install some dependencies using the old unsupported pip resolver, other dependencies are installed through the new resolver, and some other dependencies are installed through pipenv.

To solve this problem, I unified the installation of our dependencies. Now we only use pip with new resolver.

This will unlock my next contribution to install dependencies with constraints files in the PR build.

Please describe the project or issue background here

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

sfc-gh-kbregula
sfc-gh-kbregula commented Mar 8, 2023
View reviewed changes
lib/test-requirements-with-tensorflow.txt Outdated
Copy link
Contributor Author

@sfc-gh-kbregula sfc-gh-kbregula Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

keras<2.5.0 is resolved to keras==2.4.3 and tensorflow==2.6.0 requires keras~=2.6 so this requirement is invalid and always causes conflicts. We should add support for new versions of libraries, but this is only used by the st.cache decorator, which is deprecated and we have no plans to update it.

Previously this worked because we used the legacy resolver in pip, but it's not a good practice to use this as it may break other dependencies as well.
In other words, the command pip install 'keras<2.5.0' 'tensorflow>=2.6.0, <2.8.0' always fails now.

Copy link
Contributor

@sfc-gh-tszerszen sfc-gh-tszerszen Mar 9, 2023

Choose a reason for hiding this comment

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

We should get rid of the old cache decorator, point users to the new one instead and fix our dependencies 👍

@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch 3 times, most recently from 03d2607 to b1ad481 Compare March 8, 2023 14:21
@sfc-gh-kbregula sfc-gh-kbregula marked this pull request as ready for review March 8, 2023 14:21
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch 3 times, most recently from 6632dcb to 8963237 Compare March 9, 2023 15:54
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-public branch 2 times, most recently from 022a3b7 to 8b5ea08 Compare March 22, 2023 19:12
@sfc-gh-kbregula sfc-gh-kbregula changed the title Migrate from pipenv to pip only Migrate to pip only and drop pipenv Mar 24, 2023
@sfc-gh-kbregula sfc-gh-kbregula mentioned this pull request Mar 29, 2023
Merged
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch from 8963237 to ea326a9 Compare March 30, 2023 10:13
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-public branch from 8e57404 to 69381b8 Compare April 4, 2023 15:34
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch from ea326a9 to f8415bb Compare April 4, 2023 16:39
tconkling
tconkling approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@tconkling tconkling left a comment

Choose a reason for hiding this comment

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

This is great, thanks @sfc-gh-kbregula!

Makefile Outdated
Copy link
Contributor

@tconkling tconkling Apr 4, 2023

Choose a reason for hiding this comment

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

Nit: can we name this something like "INSTALL_DEV_REQS" and "INSTALL_TEST_REQS", for clarity?

Copy link
Contributor Author

@sfc-gh-kbregula sfc-gh-kbregula Apr 4, 2023

Choose a reason for hiding this comment

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

Fixed.

@kmcgrady kmcgrady added the security-assessment-completed Security assessment has been completed for PR label Apr 4, 2023
kmcgrady
kmcgrady approved these changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Approving this based on @tconkling's approval.

Next steps would be to update the Contributing Wiki Page

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Apr 5, 2023

@kmcgrady Docs updated.

@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch 2 times, most recently from 9e3388d to 7a642c4 Compare April 5, 2023 11:49
@sfc-gh-kbregula sfc-gh-kbregula changed the base branch from sfc-gh-kbregula-constraints-public to develop April 5, 2023 11:51
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-pip branch from 978dcfb to 0d9e250 Compare April 5, 2023 11:55
@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Apr 5, 2023 •
edited
Loading

I've managed to convince pipenv to still be able to use pipenv to create and manage environments, but all dependencies are installed by pip. In other words, the pipenv shell command still works for existing developers, but new developers can use python -m venv for environment management also.

@kmcgrady
Copy link
Collaborator

kmcgrady commented Apr 5, 2023

Cool. It's okay to merge.

@sfc-gh-kbregula sfc-gh-kbregula merged commit 3d11f1b into develop Apr 6, 2023
tconkling added a commit that referenced this pull request Apr 7, 2023
@tconkling
Merge branch 'develop' into st-lib
4d7157e 
* develop:
  Pass additional permission in nightly workflow (#6445)
  Release 1.21.0 (#6444)
  Typo (#6438)
  Printed / logged strings papercut fixes (#6391)
  Migrate to pip only and drop pipenv (#6265)
  Fix pillow rectangle generation in test (#6430)
@sfc-gh-kbregula sfc-gh-kbregula mentioned this pull request Apr 20, 2023
Merged
@vdonato vdonato deleted the sfc-gh-kbregula-constraints-pip branch November 2, 2023 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmcgrady kmcgrady kmcgrady approved these changes

@vdonato vdonato Awaiting requested review from vdonato

@mayagbarnes mayagbarnes Awaiting requested review from mayagbarnes

@AnOctopus AnOctopus Awaiting requested review from AnOctopus

+2 more reviewers

@sfc-gh-tszerszen sfc-gh-tszerszen sfc-gh-tszerszen left review comments

@tconkling tconkling tconkling approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sfc-gh-kbregula @kmcgrady @tconkling @sfc-gh-tszerszen