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

Make a shared requirements.txt for tests & build #105

Closed
wants to merge 1 commit into from

Conversation

KentoNishi
Copy link
Contributor

Currently, updating requirements needs to be done in two files separately. Why not just make a shared requirements.txt file for the package dependencies and install them both in CI tests? I moved the setup.py requirements list into requirements.txt which is also referenced when initializing the environment in actions.

@iver56
Copy link
Collaborator

iver56 commented Oct 18, 2021

I appreciate the idea of specifying the dependencies in a DRYer way, but I am hesitant to accept these changes. Here are my thoughts:

  1. I want to keep using conda for the dev environment, because I've never been able to install a GPU-enabled pytorch without it. I feel like conda works well, so I haven't had the motivation to "downgrade" to using only pip
  2. The package dependencies specified in setup.py must allow a wide range of dependency versions while the CI and dev environment should use pinned packages

If we should make the dependencies list DRYer, I feel like the CI should use conda and environment.yml (instead of pip and a test_requirements.txt) to set up its environment. Then we'll have two lists instead of three. But let's not boil it down to one.

@KentoNishi
Copy link
Contributor Author

fair points, ill close the issue then 👍

also I've never been able to install GPU torch through pip either so we're in the same boat 😂

@iver56
Copy link
Collaborator

iver56 commented Oct 18, 2021

Ok :) I have added an issue here: #106

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

Successfully merging this pull request may close these issues.

2 participants