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

Absolute pins on linting dependencies. #4876

Merged
merged 4 commits into from Oct 31, 2017

Conversation

Projects
None yet
5 participants
@jmchilton
Member

jmchilton commented Oct 26, 2017

  • Use completely pinned and hashed dependencies for more consistent, reproducible linting.
  • Leverage the awesomeness of pipenv to automate updating these (no more hand waving in release note checklist). pipenv is used to manage the lists of dependencies but isn't actually needed at all at runtime.
  • These will always be locked now so builds don't break randomly on other people updating their software. When we finally have the bot up and running that forward merges fixes for Galaxy we can add another if statement that runs this check against dev once a day and opens a PR if needed.

Ping @nsoranzo

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 26, 2017

Hopefully this is starting point for more use of pipenv - I'm imaging the next step is to replace dev-requirements.txt with a requirements file generated from a more complex Pipenv and Pipenv.lock for our development dependencies.

@dannon

This comment has been minimized.

Member

dannon commented Oct 26, 2017

I haven't actually tried this branch yet, but +1 to the idea here -- having been tinkering with the javascript side a ton lately, things like yarn pinning everything precisely and having a very simple 'yarn outdated', etc., command is invaluable

Absolute pins on linting dependencies.
- Use completely pinned and hashed dependencies for more consistent, reproducible linting.
- Leverage the awesomeness of pipenv to automate updating these.
- These will always be locked now so builds don't break randomly on other people updating their software. When we finally have the bot up and running that forward merges fixes for Galaxy we can add another if statement that runs this check against dev once a day and opens a PR if needed.

@galaxybot galaxybot added the triage label Oct 26, 2017

@galaxybot galaxybot added this to the 18.01 milestone Oct 26, 2017

@@ -0,0 +1,14 @@
#!/bin/sh
THIS_DIRECTORY="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

This comment has been minimized.

@nsoranzo

nsoranzo Oct 26, 2017

Member

THIS_DIRECTORY="$(dirname "$0")" should be enough here and without bashisms.

This comment has been minimized.

@jmchilton

jmchilton Oct 27, 2017

Member

Actually I think that was needed. Your variant gives me a relative path which I can't use twice. I've reworked the logic a bit in #4891's version of this file (c3776ef#diff-95de76b45ef92f3f93bdb163d284571e) so this isn't a problem though.

This comment has been minimized.

@nsoranzo

nsoranzo Oct 27, 2017

Member

You're right, but a mix of the 2 should work:

THIS_DIRECTORY="$(cd "$(dirname "$0")" > /dev/null && pwd)"
@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Oct 27, 2017

Where do we expect people to install pipenv? It has a conflicting requirement with Galaxy dependencies, namely requests>2.18.0 .

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 27, 2017

Where do we expect people to install pipenv? It has a conflicting requirement with Galaxy dependencies, namely requests>2.18.0 .

I mean I just installed it into the virtualenv but that wouldn't be best practice I don't think. I guess a standalone installation would be ideal but not needed. Any particular reason we can push up our pin of requirements that you know of?

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 27, 2017

Where do we expect people to install pipenv?

Also I don't necessarily think people need to install pipenv yet right? I'm generating backward compatible requirements.txt files for this and the other PR. You don't need pipenv to run the linting at all - just to generate the artifacts in the repo - which only needs one person to update them every month or two.

Back to the requests point - I've opened a starforge PR that updates a few various dependencies - maybe we can get a new version of some of these dependencies in Galaxy.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 27, 2017

It has a conflicting requirement with Galaxy dependencies, namely requests>2.18.0 .

Fixed in #4883 #4884.

for env in "${ENVS[@]}"
do
cd "$THIS_DIRECTORY/$env"

This comment has been minimized.

@nsoranzo

nsoranzo Oct 31, 2017

Member

For what I can see, this isn't actually updating the requirements, just creating Pipfile and requirements.txt.
Should there be a:

pipenv update

here?

This comment has been minimized.

@jmchilton

jmchilton Oct 31, 2017

Member

I was imaging more of less just keeping the Pipfile un-pinned except when it is absolutely needed (e.g. Twill, pysam, etc...) - the pinning would happen in the requirements and lock file creation here. Re-generating the lock file rebuilds a Pipfile.lock (and associated requirements files) with the newest compatible versions of the libraries in the Pipfile. If we generated new lock and requirements files and we discover they are incompatible during testing - we add version restrictions in the Pipfile and then re-run and generate the appropriate lock and requirement files.

I can't find good documentation on pipenv update and it doesn't work for me locally. I think it is for installing newer updates to the dependencies not for updating the Pipfile right? Even if it was for updating the Pipfile, I think for the above workflow it would be bad to update the locked dependencies automatically since we are only locking things in the Pipfile if we know we need to lock them.

I did notice that the Pipfile.lock files weren't added since that pattern is our .gitignore. I've added them to #4903 but I can back port them here too - but since we are still just using the generate requirements files with pip - it doesn't seem important to track the lock files at this time.

This comment has been minimized.

@nsoranzo

nsoranzo Oct 31, 2017

Member

Thanks for the explanation, I assumed that pipenv lock would create the lock file according to the versions installed in the virtualenv, not also update packages if they're not pinned.
It seems the documentation needs to be clearer on both commands.

@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Oct 31, 2017

Where do we expect people to install pipenv?

Also I don't necessarily think people need to install pipenv yet right? I'm generating backward compatible requirements.txt files for this and the other PR. You don't need pipenv to run the linting at all - just to generate the artifacts in the repo - which only needs one person to update them every month or two.

Bus factor, you know... It would be good if all committers can do that.

I mean I just installed it into the virtualenv but that wouldn't be best practice I don't think. I guess a standalone installation would be ideal but not needed.

I tried to install pipenv in my Galaxy virtualenv and it messed it up completely, had to recreate it. I ended up using:

pip install --user --upgrade pipenv

from outside of all virtualenvs.

@@ -0,0 +1,14 @@
[[source]]

This comment has been minimized.

@nsoranzo

nsoranzo Oct 31, 2017

Member

For future reference these files can be recreated with:

$ rm -rf .ci/tox_envs/flake8
$ mkdir -p .ci/tox_envs/flake8
$ cd .ci/tox_envs/flake8/
$ pipenv --rm
$ pipenv install flake8
$ pipenv lock
$ pipenv lock -r > requirements.txt
@nsoranzo

I've fixed more bashims in the script, tested with dash and committed updated Pipfile and requirements.txt for the new flake8-import-order 0.14.2 .
I've pushed everything to your branch, I hope that's OK.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Oct 31, 2017

@nsoranzo Totally fine, thanks!

@nsoranzo nsoranzo merged commit 07d11c6 into galaxyproject:dev Oct 31, 2017

6 checks passed

api test Build finished. 306 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 57 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Nov 1, 2017

Use pipenv to generate development dependencies.
- Rework pattern for linting dependencies to work for dev-requirements.txt.
- Get rid of dev-requirements.txt and replace it with a symbolic link to the latest pinned versions of dependencies.

Rebased against dev to bring in @nsoranzo's bashism bashings in galaxyproject#4876 and fixed up to strip trailing whitespace from generated requirement files.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Nov 1, 2017

Use pipenv to generate development dependencies.
- Rework pattern for linting dependencies to work for dev-requirements.txt.
- Get rid of dev-requirements.txt and replace it with a symbolic link to the latest pinned versions of dependencies.

Rebased against dev to bring in @nsoranzo's bashism bashings in galaxyproject#4876 and fixed up to strip trailing whitespace from generated requirement files.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Nov 2, 2017

Use pipenv to generate development dependencies.
- Rework pattern for linting dependencies to work for dev-requirements.txt.
- Get rid of dev-requirements.txt and replace it with a symbolic link to the latest pinned versions of dependencies.

Rebased against dev to bring in @nsoranzo's bashism bashings in galaxyproject#4876 and fixed up to strip trailing whitespace from generated requirement files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment