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

Dist removal #5074

Merged
merged 66 commits into from
Jan 26, 2024
Merged

Dist removal #5074

merged 66 commits into from
Jan 26, 2024

Conversation

mbargull
Copy link
Member

@mbargull mbargull commented Nov 15, 2023

Description

Work in progress to remove legacy conda.models.dist.Dist usage.
Excerpt from gh-5069 :

[...] conda-build's slow (and memory intensive) ramp up before it gets to its actual first environment solving.
[...] work on purging legacy behavior/code such that we could at least prepare to make use of the deferred creation of package records.
A first step in that direction would be to finally remove conda-build's usage of the old Dist objects (removed from conda's core workings in 2018).


Impact

  • Prepare usage of newer/more performant package index handling in conda-build.
  • Removal and possible removal tech debt not only in conda-build but also conda.
  • Breaking changes for library uses of conda_build:
    • not only might we remove Dist-related functions which might be used downstream,
    • but also not using a Dist-based package index anymore could break downstream expectations.

Closes gh-5069

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Nov 15, 2023
@mbargull
Copy link
Member Author

mbargull commented Nov 15, 2023

Pull request overiew:

Goal

  • Change necessary parts to remove the usage of Dist.
  • Do not change how environment creations happen just yet.
    => This means: Actual runtime benefits (time/memory reduction) should be done after the Dist removal/cleanup work. (Ideally in subsequent PRs.)

Strategy

  • Do not change behavior.
  • Work iteratively and surgically.
  • Copy over conda.models.dist and conda.plan (+conda.instructions).
    (These still exist (nearly) completely for conda-build only and could otherwise be removed from conda.)
  • Iteratively reduce the surface of Dist/plan interaction.
    • remove Dist usage and unneeded functionality from Dist and plan
      (e.g., conda-build only creates, never updates environments => only create/link parts of plan are actually used)

Progress

  • Dist objects have been removed.
  • Test suite not yet run (hence opening this draft PR).
    • (Expecting failures; only ran local conda-forge/zlib-feedstock builds not yet more of the test suite, yet.)
  • Removed most unneeded/defunct code regarding plan.

To do

  • More cleanup needed.
  • Better document the changes (i.e., fix commit messages to be more descriptive)

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
mbargull and others added 3 commits January 26, 2024 12:29
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
The replaced code did not rstrip, so let us keep it that way for now.

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
@mbargull
Copy link
Member Author

FYI I just opened #5151 to remove all 24.1.0 deprecations

Thanks, I'll merge this gh-5151 here once it's in.

@jezdez jezdez changed the title [WIP] Dist removal Dist removal Jan 26, 2024
jezdez
jezdez previously approved these changes Jan 26, 2024
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
CHANGELOG.md Outdated Show resolved Hide resolved
kenodegard
kenodegard previously approved these changes Jan 26, 2024
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

looking good! 🚀

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>
CHANGELOG.md Show resolved Hide resolved
@mbargull
Copy link
Member Author

Thanks everyone for reviewing!

@kenodegard
Copy link
Contributor

kenodegard commented Jan 26, 2024

@mbargull phew! thanks soooo much for working on this (and being patient with the slooooow CI)! we would not have gotten this done nearly as quickly if you hadn't helped out with the Dist removal

BTW since you have merge rights can you click auto-merge and update the squashed commit message?

@mbargull mbargull enabled auto-merge (squash) January 26, 2024 21:38
@mbargull
Copy link
Member Author

(and being patient with the slooooow CI)

All good :).
I was also surprised about the skeleton test failure and had to dig a bit to understand/remember those permission issues (previously encountered those some years ago with clean up code for go packages which marked directories as read-only).
At some point I'd also like to continue to look into the parts in conda-build that make it so slow on Windows/macOS so we may get sub 30min CI times on all platforms 🤞 :).

BTW since you have merge rights can you click auto-merge and update the squashed commit message?

Oh right, good idea! Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove conda.models.dist.Dist usage
4 participants