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

Use pip-tools to lock versions of dependent packages #2093

Merged
merged 5 commits into from
Dec 7, 2021
Merged

Use pip-tools to lock versions of dependent packages #2093

merged 5 commits into from
Dec 7, 2021

Conversation

ysk24ok
Copy link
Contributor

@ysk24ok ysk24ok commented Nov 26, 2021

What this PR does / why we need it:

This PR introduces pip-tools to lock the version of dependent packages.
Other options are Poetry and Pipenv.
Using Poetry, we have to replace setup.py with pyproject.toml, which leads to comparatively large diff. That's why I didn't choose Poetry.
At first I tried using pipenv but decided not to use it based on maintainers' feedback. pipenv has some drawbacks such as slow locking and we don't need its virtualenv management feature right now.

Several changes to note are the followings.

  • Adding py-3.X-ci-requirements.txt files. The result of pip-compile does not change between Linux and MacOS so the OS specifier is not included in the file name.
  • Using py-3.X-ci-requirements.txt as a cache key instead of setup.py.

Which issue(s) this PR fixes:

Fixes #1993

Does this PR introduce a user-facing change?:

NONE

@feast-ci-bot
Copy link
Collaborator

Hi @ysk24ok. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Yusuke Nishioka <yusuke.nishioka.0713@gmail.com>
Signed-off-by: Yusuke Nishioka <yusuke.nishioka.0713@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #2093 (56751ee) into master (54d0f3a) will decrease coverage by 25.44%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2093       +/-   ##
===========================================
- Coverage   83.44%   57.99%   -25.45%     
===========================================
  Files         100      100               
  Lines        8077     8088       +11     
===========================================
- Hits         6740     4691     -2049     
- Misses       1337     3397     +2060     
Flag Coverage Δ
integrationtests ?
unittests 57.99% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../integration/online_store/test_universal_online.py 15.34% <0.00%> (-82.80%) ⬇️
.../integration/online_store/test_online_retrieval.py 17.39% <0.00%> (-82.61%) ⬇️
...fline_store/test_universal_historical_retrieval.py 19.29% <0.00%> (-80.12%) ⬇️
sdk/python/tests/utils/online_read_write_test.py 20.68% <0.00%> (-79.32%) ⬇️
...gration/registration/test_feature_service_apply.py 31.25% <0.00%> (-68.75%) ⬇️
sdk/python/feast/infra/online_stores/redis.py 26.97% <0.00%> (-67.11%) ⬇️
sdk/python/tests/data/data_creator.py 34.78% <0.00%> (-65.22%) ⬇️
sdk/python/tests/integration/e2e/test_usage_e2e.py 35.00% <0.00%> (-65.00%) ⬇️
...s/integration/registration/test_universal_types.py 36.00% <0.00%> (-64.00%) ⬇️
...istration/test_universal_odfv_feature_inference.py 38.29% <0.00%> (-61.71%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d0f3a...56751ee. Read the comment docs.

@ysk24ok ysk24ok changed the title Use pipenv Use Pipenv to lock versions of dependent packages Nov 26, 2021
@ysk24ok ysk24ok marked this pull request as ready for review November 26, 2021 07:59
@adchia
Copy link
Collaborator

adchia commented Dec 1, 2021

More of a general question / comment. I haven't messed around personally with pipenv. The biggest problem I see right now is with dependency resolution in Feast.

Seems like generally, people have very mixed feelings about pipenv and that poetry is the strongly preferred alternative. pipenv seems like overkill for that (and using pip-tools / pip-compile might be more useful for this). poetry I could be convinced. Do you have stronger thoughts on this?

@ysk24ok
Copy link
Contributor Author

ysk24ok commented Dec 2, 2021

Seems like generally, people have very mixed feelings about pipenv and that poetry is the strongly preferred alternative.

Yes, poetry is a better solution these days. Time for dependency lock is less and it can handle packaging by poetry build and poetry push commands.
The reason I didn't choose poetry is that I have to replace setup.py with pyproject.toml. It's too much, at least for now, because what we want to solve is dependency conflict.
I chose pipenv because I'm familiar with it and I thought it would take less time to create this PR.

pipenv seems like overkill for that (and using pip-tools / pip-compile might be more useful for this). poetry I could be convinced.

I also think pip-tools is a simple enough solution for dependency conflict. Replacing pipenv with pip-tools won't take that long.
Which do you prefer, pip-tools or poetry? After you decide, I'll work on replacing pipenv with it.

@achals
Copy link
Member

achals commented Dec 3, 2021

@ysk24ok Is the idea with this diff that we'd pipenv while developing feast so that dev environments use a consistent version of dependent packages? Or would this affect how end users installed feast, when they run pip install feast?

@adchia
Copy link
Collaborator

adchia commented Dec 3, 2021

my personal preference likely is to use pip tools to start

@ysk24ok
Copy link
Contributor Author

ysk24ok commented Dec 3, 2021

@achals
Both.
Tools like pipenv try to lock dependencies based on install_requires and extras_require in setup.py, and users also try to install Feast by running pip install feast based on them.
If the lock fails, it means dependency version specification in setup.py is wrong and developers have to fix it. It leads no failure around dependencies for users in installation.

@adchia
Understood. I'll replace pipenv with pip-tools.

This reverts commit e38eddf.

Signed-off-by: Yusuke Nishioka <yusuke.nishioka.0713@gmail.com>
This reverts commit da9c425.

Signed-off-by: Yusuke Nishioka <yusuke.nishioka.0713@gmail.com>
Signed-off-by: Yusuke Nishioka <yusuke.nishioka.0713@gmail.com>
@ysk24ok
Copy link
Contributor Author

ysk24ok commented Dec 6, 2021

@adchia Replaced pipenv with pip-tools (pip-compile/pip-sync). Could you take a look?

Several changes to note are the followings.

  • Adding py-3.X-ci-requirements.txt files. The result of pip-compile does not change between Linux and MacOS so the OS specifier is not included in the file name.
  • Using py-3.X-ci-requirements.txt as a cache key instead of setup.py.

@adchia adchia changed the title Use Pipenv to lock versions of dependent packages Use pip-tools to lock versions of dependent packages Dec 6, 2021
@adchia
Copy link
Collaborator

adchia commented Dec 6, 2021

Asking achal to take a look since he's had experience with pip-tools!

@achals
Copy link
Member

achals commented Dec 6, 2021

@ysk24ok The PR looks pretty good! Can you update the description so that it matches the new approach?

@ysk24ok
Copy link
Contributor Author

ysk24ok commented Dec 6, 2021

@achals Thank you. I've updated the description.

Copy link
Member

@achals achals left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, ysk24ok

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Managing dependencies using tools like pipenv or poetry
6 participants