Skip to content
This repository has been archived by the owner on Jan 24, 2018. It is now read-only.

Dependency management - relaxing requirements #1557

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

david4096
Copy link
Member

@david4096 david4096 commented Feb 7, 2017

  • Relax some version requirements (== becomes >=). A minor change to work with pysam 0.10.0 was made.

  • I haven't seen many projects use the pipedeptree approach to defining dependencies. I understand that these packages should be able to maintain their own dependency tree, and we don't need to (and shouldn't) try to manage their dependencies for them. Section removed.

  • Remove the humanize dependency, which was used for one line on the landing page (server uptime nicely printed).

  • The lxml requirement is only needed for testing https://github.com/ga4gh/server/blob/73e496bc2995a4066ca35411c6e4ac1c7097ea77/tests/end_to_end/test_oidc.py .

  • sphinx-argparse is needed in RTD, but not for basic server usage.

Note: I did not relax the requirements for Flask as there appear to be some problems with how we are using the flask configuration for logging. A future PR will update our dependencies for Flask and Flask_cors.

Note, this will keep our software better up to date as CI will be able to find when one of our dependencies triggers a breaking upgrade. During release, it is possible to once again fix the requirements to a known working version of a module.

@codecov-io
Copy link

Codecov Report

Merging #1557 into master will not impact coverage by -0.01%.

@@            Coverage Diff             @@
##           master    #1557      +/-   ##
==========================================
- Coverage   84.62%   84.61%   -0.01%     
==========================================
  Files          33       33              
  Lines        7250     7247       -3     
  Branches      911      911              
==========================================
- Hits         6135     6132       -3     
  Misses        948      948              
  Partials      167      167
Impacted Files Coverage Δ
ga4gh/server/frontend.py 90.99% <ø> (-0.07%)

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 b67a723...34365fa. Read the comment docs.

Copy link
Member

@dcolligan dcolligan left a comment

Choose a reason for hiding this comment

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

For the discussion behind the rationale of why things were done this way, see here: #1007

The long and short of it is, if we don't pin dependencies, those dependencies (or dependencies of dependencies, etc.) can push a change that breaks our code. The "downside" of pinning is that we don't get the latest versions of that code. But since I would favor stability over the often questionable advantage of getting the most up-to-date dependency code, I implemented it this way.

Another downside of this change is that we are potentially introducing another (complicated and easy to screw up) step before releases involving pinning the appropriate packages.

I put a very high premium on package stability. If we are making a decision to not be so maniacal about that, then fine, but I want us to make that decision with an appreciation of the implications.

Sphinx==1.4.6
sphinx_rtd_theme
sphinx-argparse==0.1.15
Copy link
Member

Choose a reason for hiding this comment

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

Why was sphinx-argparse moved into dev-requirements? Will the builds on RTD still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that should be in requirements? I consider the doc creation as the development dependencies. We can manage RTD build dependencies in a nicer way using the environment.yml. However, this PR is untested on RTD.

Copy link
Member

Choose a reason for hiding this comment

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

Er, the client has sphinx-argparse in dev-requirements.txt, so moving it seems fine

Copy link
Collaborator

@ejacox ejacox left a comment

Choose a reason for hiding this comment

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

I agree with @dcolligan that stability is important. On the other hand, I am slightly curious to see if we have any problems if don't pin all the versions.

@david4096
Copy link
Member Author

david4096 commented Feb 8, 2017

Thanks both! And thanks for the link @dcolligan. We definitely want to strike a balance between keeping our code base active and delivering packages we trust will run.

I think that the approach of adding pipedeptree is nice for giving better guarantees, and it might make sense to make that part of the release process. I think that CI makes this situation easier to maintain, a nightly build failure will let us know which dependency updated. This is a better developer position to be in than guessing which modules should receive updates.

If we are using modules in ways that break when their dependencies change, we should try to remove those bits of code. Those are like private API calls and we want the testing machinery to reveal them. By relaxing the requirements we might get some more information about how to maintain the software. Not to mention bug fixes and performance improvements in our dependencies.

Do you think we could try a cycle or two with pipedeptree removed and see if we get any spurious problems? Managing dependencies in python is a nightmare so I think in practice many people will dockerize their environment anyway.

@dcolligan
Copy link
Member

Saying this is hurting the cloud software portion of my soul, but sure, we can try the relaxed requirement strategy. Having the nightly CI builds at least gives us worst-case 24 hours notice of a problem. On the other hand, we will probably have to make more bugfix releases this way to respond to problems with dependencies shipping versions that break our code.

@kozbo
Copy link
Contributor

kozbo commented Feb 9, 2017

l like the idea of pinning our dependencies at release time for the very reason that @dcolligan expressed. We should know which versions we work with and then enforce them on our releases to assure that our installations work as promised.

@david4096
Copy link
Member Author

I'll leave it up to y'all! From my position it's information we're not getting because our dev version has the versions pinned for all it's requirements. I understand it's a little more work for the release. You'd run pip list and set the version for any packages you want to set.

From @dcolligan in #1007

I know that this strategy does have downsides, including not getting the most updated packages, with their bugfixes and features. However, this is a cost that I am willing to pay, and -- more to the point -- it's not the priority.

Also, worth noting that you point out a couple of bad apples. yubico-client, pyjwkest are both modules that are part of Lshift's OIDC module, yubico being just for tests. I think this might be a case of having to overthink things because of these packages. I want to (automatically) know which packages aren't being managed well, so we can deprecate them.

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

Successfully merging this pull request may close these issues.

5 participants