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

Feat/pip tools #235

Merged
merged 5 commits into from
Jul 6, 2020
Merged

Feat/pip tools #235

merged 5 commits into from
Jul 6, 2020

Conversation

BenjaSanchez
Copy link
Contributor

@BenjaSanchez BenjaSanchez commented Jul 3, 2020

Main improvements in this PR:

A problem with storing python dependencies as requirements.txt is that there's no way of distingushing between actual dependencies and transitive dependencies (i.e dependencies of the actual dependencies). For that purpose, Here we switch to use pip-tools: All original dependencies are stored in requeriments.in, and using pip-compile we generate the requirements.txt file to be pip install'ed (or even pip-sync'ed via pip-tools). Additionally, dev-requirements.in is provided for developers (same as the other file but also with pip-tools). This scheme also allows even more environments to be implemented later on (for instance ci-requirements.in for the memote implementation, so that it only installs the minimum packages for the test suite).

@Midnighter I borrowed this structure from some of the DeCaF microservices, let me know if it makes sense to you. @hongzhonglu any other useful package that users need and I'm forgetting in requirements.in?

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Selected devel as a target branch (top left drop-down menu)

to easily keep track of the original requirements. Also build requirements.txt from requirements.in and adapt README accordingly.
same as user requirements but also with pip-tools
@BenjaSanchez BenjaSanchez added the enhancement new field/feature label Jul 3, 2020
@BenjaSanchez BenjaSanchez self-assigned this Jul 3, 2020
@BenjaSanchez
Copy link
Contributor Author

@mihai-sysbio would this scheme fit with the standard-GEM standard? Have you discussed how to manage dependencies?

README.md Outdated Show resolved Hide resolved
@Midnighter
Copy link

Looks good to me! Two suggestions:

  1. I would stick to pip install -r rather than pip-sync because syncing will remove anything in the environment not mentioned in the file. This can have annoying consequences.
  2. If you don't want to re-list dependencies, you can refer to other requirements files. For example,

You have your requirements.in and requirements.txt. Then in dev-requirements.in you start with:

-r requirements.txt

...

This will use the pinned versions of your basic requirements. Sometimes you may need to play with the path, i.e., it might need to be -r requirements/requirements.txt.

@mihai-sysbio
Copy link
Member

Another thought I have is related to the current identity between requirements.in and dev-requirements.in. The dev- looks easy to add anytime, so unless it provides value right now I would consider not adding it. This is a preference of mine to try to keep things as simple as possible for as long as possible.

@BenjaSanchez
Copy link
Contributor Author

BenjaSanchez commented Jul 3, 2020

@Midnighter

  1. Noted, although I can imagine advantages of pip sync'ing when trying to reproduce errors from users? In any case the installation instructions in README.md are only with pip install -r.
  2. Good point! Done in 02032da.

@mihai-sysbio

In fact it already provides me with value, as I've been switching between my yeast-gem and yeast-gem-dev environments depending on my needs (working with the model / updating packages). Also, it frees users from having the pip-tools dependency :) After 02032da hopefully the identity looks clearer? I also updated CONTRIBUTING.md (fcd8771) to clarify how to modify each file.

@mihai-sysbio
Copy link
Member

@BenjaSanchez coming back to your original question on dealing with requirements in standard-GEM, there is no plan regarding managing dependencies, which means for now it's up to each repo to manage. Somehow, I feel that the requirements pertain to code, so they would sit in the code folder.
Come to think of it, one idea to not ask users to create a blank .env would be to create a PR against master that would add that file there, without having to add it to develop.

@BenjaSanchez
Copy link
Contributor Author

@mihai-sysbio currently we require the .env file also in devel, as we rely on dotenv to find the repo's root when using read_yeast_model and write_yeast_model, following this reproducible research project standard. However, I'm ok with prescinding of that with an alternative implementation out there, e.g. using the location of the .git folder instead.

@mihai-sysbio
Copy link
Member

standards 👍
The addition of requirements.txt and .env at a top level give a distinct Python flavour to a project, which I am unsure is part of the philosophy in the original rr-init repository. Anyway, as long as the idea is to standardize, I am supportive of that.

@BenjaSanchez BenjaSanchez merged commit 5d0d311 into devel Jul 6, 2020
@BenjaSanchez BenjaSanchez deleted the feat/pip-tools branch July 6, 2020 08:46
@BenjaSanchez BenjaSanchez mentioned this pull request Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new field/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants