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

[do not merge] major refactor of downloads, environment creation #107

Closed

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Nov 21, 2020

As a prelude to #75/#76, we talked about splitting out the downloader pieces into smaller things.

This is that... and entirely too much else. I basically couldn't track where all the if statements were going, and this is what I came up with to make future providers of installers more tractable... micromamba wouldn't look like what's in there, but we'll get there when we get there, I guess.

  • move most of the validation to parse time to fail earlier
    • more can be done here
  • strategy pattern (one per very short file, except miniconda download) applied to:
    • installers sources
    • base tools (e.g. mamba, conda-build)
    • environment creation
    • baseline packages (e.g. python, though now can add more)
  • does at most one solve, seems quite snappy
  • more consistent top-level logging approach
  • standardized most functions to take...
    • a frozen inputs (as from the yaml, this could still be improved
    • a dynamic grab-bag of things (e.g. useMamba, the parsed env file)
      • some more typing could be applied here
  • normalize package spec string creation
  • removed most of the any types
  • removed the (Error|Success)Result pattern, leaning almost entirely on throw and one top-level catch (except around settings)

Anyhow... I would worry about this bitrotting pretty fast, as any other features/fixes change, but it's also going to be a bear to review. So sorry! I can try to pull it apart into smaller incremental pieces, but basically did the whole thing in one go when my network was patchy.

Sub-PR Plan

* major refactor of downloads, environment creation
@goanpeca
Copy link
Member

goanpeca commented Nov 21, 2020

Hi @bollwyvl as always thanks for this. This is a bit too much for a single PR so if we could split this, that would be great. Also less chance of breaking things.

Question what is the deal with underscored files?

@bollwyvl
Copy link
Contributor Author

Question what is the deal with underscored files?

Just for sorting. could be changed.

too much for a single PR

Sure, fair. Will add a top-level plan of action...

@bollwyvl bollwyvl marked this pull request as draft November 24, 2020 13:05
@bollwyvl bollwyvl changed the title major refactor of downloads, environment creation [do not merge] major refactor of downloads, environment creation Nov 24, 2020
bollwyvl added a commit to bollwyvl/setup-miniconda that referenced this pull request Nov 29, 2020
goanpeca added a commit that referenced this pull request Dec 18, 2020
bollwyvl added a commit to bollwyvl/setup-miniconda that referenced this pull request Dec 19, 2020
bollwyvl added a commit to bollwyvl/setup-miniconda that referenced this pull request Dec 19, 2020
bollwyvl added a commit to bollwyvl/setup-miniconda that referenced this pull request Dec 19, 2020
bollwyvl added a commit to bollwyvl/setup-miniconda that referenced this pull request Dec 19, 2020
goanpeca pushed a commit that referenced this pull request Dec 19, 2020
* use native throw instead of Result

* refactor input parsing and pre-validation

* remove errant empty file
@goanpeca goanpeca added this to the v2.1.0 milestone Dec 19, 2020
goanpeca pushed a commit that referenced this pull request Dec 20, 2020
* adopt dynamic options pattern, with temporary condaConfig

* collapse additional type imports
goanpeca pushed a commit that referenced this pull request Dec 24, 2020
* move variable and step output to common outputs

* refactor more conda pieces, clean up imports

* extract env parsing and installation to dedicated files

* sweep of comment capitalization, add some comments, shouty-case provider lists
goanpeca pushed a commit that referenced this pull request Dec 24, 2020
* move base env tool providers to dedicated files

* add some more log output for mamba bash

* more work on mamba log output

* use namespace import for constants, fix comment typos

* fix windows path in mamba wrapper, add explit error throwing

* ignore channel config warning, add a canary for the timestamp in log
@bollwyvl bollwyvl mentioned this pull request Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants