-
Notifications
You must be signed in to change notification settings - Fork 354
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
Bump to recent versions, and make bootstrap.py update to those when run #719
Bump to recent versions, and make bootstrap.py update to those when run #719
Conversation
86869a7
to
d38f6db
Compare
d38f6db
to
e966f64
Compare
4cdd45b
to
1ea9115
Compare
072154e
to
cf24256
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. I'll defer to @GeorgianaElena on the chardet question.
For fully frozen environments, it would be best to use pip-tools or conda lockfiles to separate the 'actual' loosely defined dependencies from the fully frozen ones. I think it's good to move away from manually managed exact pins.
044cfec
to
e9b9747
Compare
c0671e2
to
15be1c1
Compare
15be1c1
to
1dab737
Compare
@yuvipanda @minrk I've rebased this PR on #721 now - could you re-review it? Since @minrk this PR approved, I've added the commit 1dab737 that makes us do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go once the tests pass
@yuvipanda I did To clarify, my assumption is that if I do Conda flags details
|
I'm thinking that either we want to have I think EDIT: aaargh well it seems like dependencies are updated by default. I'm not changing conda to update anything explicitly with a flag. |
1dab737
to
536b055
Compare
Since we now longer pin versions to the patch version, we should make an install cause the packages upgrade within the version constraints rathern than just settle for the current version if it is already installed.
536b055
to
125e12c
Compare
Final summary@yuvipanda, I made |
tljh/conda.py
Outdated
'install', | ||
'--no-cache-dir', | ||
] + packages) | ||
pip_cmd = pip_executable + ['install', '--no-cache-dir'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but we we probably don't want to call pip with --no-cache-dir
. This is common as a space optimization for keeping images small, but isn't the right thing for any pip install on a persistent VM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh note I retained --no-cache-dir
, but I can remove it part of this PR, makes sense to me to remove!
tljh/conda.py
Outdated
'-r', | ||
requirements_path | ||
]) | ||
pip_cmd = pip_executable + ['install', '--no-cache-dir'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, now that I see you've added --no-cache-dir
here, let's not do that. And you can remove it above, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you are right, I added it here! Woops!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment that we shouldn't use --no-cache-dir
with pip, but LGTM!
With recent approvals (and fix of removing --no-cache-dir), ill go for a merge on this! Thank you @minrk and @yuvipanda for the review efforts!! |
PR summary
I saw quite a bit of very specific pinnings, all the way to the patch version. I figure that is problematic so I made it instead pin the latest major version, or in case no major version has been released to the first minor version.
I updated:
pip install oauthenticator==1.*
should include the --upgrade flag - right? #732 introduced by not pinning strictly any more--upgraded
added topip install
--update-deps
and--update-spec
added toconda install
My intention is that this should update the package and its dependencies, just like I expect
pip install --upgrade
works.Related
pip install oauthenticator==1.*
should include the --upgrade flag - right? #732