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

WIP: Relax Python Dependencies #954

Closed
wants to merge 2 commits into from

Conversation

SwampertX
Copy link
Collaborator

What this PR does / why we need it:
Feast's Python SDK has dependencies that clash with popular data science libraries, like pandas and pyyaml. This PR aims to fix that, and perhaps find a most suitable way to do this.
Which issue(s) this PR fixes:

Fixes #928

Does this PR introduce a user-facing change?:

Update Python SDK Dependencies

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SwampertX
To complete the pull request process, please assign woop
You can assign the PR to them by writing /assign @woop in a comment when ready.

The full list of commands accepted by this bot can be found 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

@SwampertX
Copy link
Collaborator Author

The current update is done by

  1. Removing all the restriction in dependency versions, in sdk/python/setup.py
  2. pip install -e sdk/python in a new virtual environment.
  3. Extract the dependent packages' versions from pip freeze.
  4. Update sdk/python/setup.py and requirements-*.txt with latest versions that does not conflict with tensorflow==2.3.0
  5. Use >= for all the required packages.

Appreciate comments on what is the best practice here.

@feast-ci-bot
Copy link
Collaborator

@SwampertX: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
test-end-to-end-batch fb515d5 link /test test-end-to-end-batch
test-end-to-end-batch-dataflow fb515d5 link /test test-end-to-end-batch-dataflow
test-end-to-end-auth fb515d5 link /test test-end-to-end-auth
test-end-to-end-redis-cluster fb515d5 link /test test-end-to-end-redis-cluster
test-end-to-end fb515d5 link /test test-end-to-end

Full PR test history

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. I understand the commands that are listed here.

"numpy",
"google",
"confluent_kafka",
"click>=7.1.2",
Copy link
Member

@woop woop Aug 25, 2020

Choose a reason for hiding this comment

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

Are these really the lowest versions we can use? (everything in REQUIRED)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are the latest versions we can use instead. This way the dependencies will not be outdated for longer, but is likely going to conflict with old packages. Although no conflicts are detected when I last tested the dependencies by installing in a new virtualenv with tensorflow==2.3.0.

Copy link
Member

Choose a reason for hiding this comment

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

Yea but basically, if somebody is running Pandas 1.0.0 then Feast wont work for them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SwampertX I agree with @woop, the dependency requirements should be as low as possible since many of our existing users are using older version of library.

@woop woop closed this Oct 27, 2020
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.

Transitive dependencies are too specific in install_requires
4 participants