-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make conda env update --prune
ignore history specs.
#9614
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
Great work @tarcisioe! Fixing |
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
@marcelotrevisani @msarahan I have submitted my CLA (twice, actually), is anything else needed for the bot to see it? Maybe it needs to be updated in .cla-signers? |
Also, I believe appveyor/pr wasn't broken by my change, and I'm not too sure about what's happened in circleci conda-build. Should I do something about those? |
Hi @tarcisioe , this process has a manual part lets say. @msarahan has to review and approve it. It seems to be a legal process which they cannot avoid. |
@@ -363,6 +363,170 @@ def test_prune_1(tmpdir): | |||
assert convert_to_dist_str(link_precs) == link_order | |||
|
|||
|
|||
def test_update_prune_1(tmpdir): |
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.
Thanks for the tests! :)
@cla-bot check |
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
The cla-bot has been summoned, and re-checked this pull request! |
what's the status of this? does the maintainer agree that this is the right fix? |
I tried to manually apply this patch to 4.8.2 and the steps in #7279 (comment) is still broken |
@kahing It hasn't been reviewed yet by the maintainers. I will try to reproduce the steps you posted and investigate. |
Well, shame on me I guess. I developed this patch on top of 4.7.x and trusted my regression tests. They are indeed still passing, so something else reintroduced the issue. I went through the major tags rebasing my patch onto them:
So I made a script that applied my patch and tested, and got the change that ended up nullifying my fix with |
8be1767
to
c7451e2
Compare
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
@kahing it's now rebased on master and when pruning the option "freeze-installed" is ignored (which was making packages that should otherwise be pruned be kept). I'm unsure if that is the best solution, but it basically reverts prune to the previous behaviour. Another regression test was added. |
Thanks for contributing this PR, and apologies for the delay in review. We're undergoing some changes in the Conda team right now, but it's our plan to be spending considerably more resources directly on Conda soon. In particular for this PR, does this PR change the behavior of |
@kalefranz It shouldn't. What it does is:
Therefore, Also, no issues with the delay, I am aware of the size of conda as a project and how much work you guys have. I'm the one to thank you for the maintenance! |
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
01502fc
to
c7451e2
Compare
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
The current state can trigger an error due the missing installed specs. |
c7451e2
to
463c94c
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Martin Prüsse.
|
463c94c
to
2ae84f0
Compare
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @tarcisioe. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
2ae84f0
to
7bd6701
Compare
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
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 like it is passing 🎉, just need the news snippet and then this will be good to go
update --prune
ignore history specs.
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.
TIL that --prune
is only enabled in conda env
. I had the impression that it was a general flag (and the code in solve.py
does not check for the subcommand used), but it's not exposed in any other CLI. 🤷
For that reason, I'd change the PR title to "Make conda env update --prune
ignore history specs"? Although the tests seem to be general and could be extended to conda update
if the CLI was updated.
update --prune
ignore history specs.conda env update --prune
ignore history specs.
* start branch * trigger ci * port logic from conda/conda#9614 * deselect a couple tests (accelerate and libmamba do not get along) * add news * Apply suggestions from code review * Alternative logic for solver creation tasks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix tests/core/test_solve.py::test_auto_update_conda * some progress with pins (but not quite there yet) * Increase timeouts in CI env setup * allow uninstall by default only on removals * handle prune * avoid crash * cleanup * only lock the installed spec * pre-commit * add SolverInputState.always_update helper mapping * allow uninstall of conflicts * use always_update list for simplicity * not update-all if pinned * explain pins if involved in errors * re-enable original test_neutering_of_historic_specs * bump minimum required conda and libmamba versions * fix tests/core/test_solve.py::test_force_remove_1 and tests/core/test_solve.py::test_pinned_1 * Re-enable tests that are failing * Require libmambapy >= 1.5.1 * pre-commit * amend news * Update __init__.py * fix test_neutering_of_historic_specs * consider 1.5.1 in warnings too * pre-commit * adjust test * remove comment * fix json serialization * xfail in 1.5.1 too * catch RuntimeError for the whole 1.5.x series * xfail test_explicit * fix test_pinned_1 * lock installed name-only pinned packages * do not add installed channels to list if --override-channels in use * add adjusted variant for test_priority * always define lock * explain some deselected tests * adjust test_install_features * xfail test_export * pre-commit --------- Co-authored-by: Jannis Leidel <jannis@leidel.info> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Chris Ostrouchov <chris.ostrouchov@gmail.com>
This is supposed to solve #7279. The idea here is that if we are pruning we must use only the specs coming from environment.yml and ignore the history. Regression tests have also been added.
Resolves #7279