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

Lock setup dependencies #1458

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Lock setup dependencies #1458

merged 1 commit into from
Jul 13, 2021

Conversation

jgwerner
Copy link
Contributor

Locks setup dependencies to patch versions.

Signed-off-by: Greg Werner <[email protected]>
@BertR BertR added the dependencies Python dependencies label Jun 16, 2021
@BertR BertR requested a review from jhamrick June 17, 2021 07:50
@BertR BertR merged commit 4c7e174 into jupyter:master Jul 13, 2021
@rkdarst
Copy link
Contributor

rkdarst commented Feb 8, 2022

I'm wondering what was the motivation for locking all dependencies? I normally strongly discourage anyone distributing packages from doing this, for the reasons here: https://scicomp.aalto.fi/scicomp/packaging-software/#don-t-pin-dependencies

Practically... how do people usually deal with this? I have someone working with me on nbgrader work now, and I was a bit surprised to see this since I remember it being more specific about problem dependencies before. Except for this pull request diff, how does someone which of these requirements are real, vs needed for one particular deployment? How does one co-install with other things which have a different update cycle? (well, the practical answer is "pip ignores dependency versions", which sort of shows how locking versions isn't really needed anyway...)

I clearly have opinions here, but I'm wondering what others tend to do. I would definitely support a separate lockfile or some such that says "these versions work". Also, practically speaking, it makes more sense for nbgrader rather than other projects, since it's not actively maintained, so maybe it needs to be locked, because we know it's going to break someday anyway, and no one will be around to fix it quickly.

rkdarst added a commit to AaltoSciComp/nbgrader that referenced this pull request Feb 8, 2022
- Follow-up to jupyter#1458, which locked all dependency versions.
- This adds a comment which indicates what the actual last-known
  requirements are.  This may be important to anyone working to update
  these in the future.
- Review: think about the meaning of the locks and if there is better
  way to represent "working environment" vs "dependencies".
@perllaghu
Copy link
Contributor

The primary reason, from my point of view, is to ensure that the application works as intended.

The other version is that up-stream updates can change APIs, and break services..... witness the fun NBGrader had with NBConvert - when it changed from 5.6.1 to 6.0.0

@BertR
Copy link
Contributor

BertR commented Feb 8, 2022

I clearly have opinions here, but I'm wondering what others tend to do. I would definitely support a separate lockfile or some such that says "these versions work". Also, practically speaking, it makes more sense for nbgrader rather than other projects, since it's not actively maintained, so maybe it needs to be locked, because we know it's going to break someday anyway, and no one will be around to fix it quickly.

@rkdarst : I think your last paragraph hits a good point, this was done because we wanted to make sure there was a working branch, and new CI runs were failing because of broken dependencies.
I we could do some work to establish the range of working versions, we could relax the pinning of the dependencies. eg. notebook<7.0.0. For some packages that are fairly stable (requests, tornado) we can consider removing the pins altogether.

Thoughts @rkdarst , @jgwerner & @perllaghu ?

@jgwerner
Copy link
Contributor Author

jgwerner commented Feb 8, 2022

@BertR I would agree that we need a range of working versions to avoid conflicts. I have something locally that accomplishes this requirement. I haven't updated the requirements in a few weeks so will review the packages and send a PR for review.

@jhamrick jhamrick added this to the 0.7.0 milestone May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Python dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants