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

Update micromamba installation process, refactor options #185

Merged
merged 40 commits into from
Jan 23, 2024

Conversation

0xbe7a
Copy link
Contributor

@0xbe7a 0xbe7a commented Dec 15, 2023

This pull request updates the micromamba installation process to use a pre-installed binary if available and micromamba-binary-path and download-micromamba are undefined or false.

@0xbe7a 0xbe7a requested a review from pavelzw as a code owner December 15, 2023 13:00
@pavelzw pavelzw added the enhancement New feature or request label Dec 15, 2023
@pavelzw
Copy link
Member

pavelzw commented Dec 15, 2023

Do we need new test cases?

Also, can you bump the version to 1.8.0 in package.json?

Copy link
Collaborator

@jonashaag jonashaag left a comment

Choose a reason for hiding this comment

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

This is getting a bit complicated... my hypothesis how it works

Exists in PATH download-micromamba micromamba-binary-path Behavior
No false unset Error
No false set Use binary (absolute path)
No true unset Download to default location
No true set Download to custom location
Yes false unset Use binary (from PATH)
Yes false set ?
Yes true unset ?
Yes true set ?

src/options.ts Show resolved Hide resolved
@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 15, 2023

Micromamba on $PATH download-micromamba micromamba-binary-path Behavior
No unset unset Download to default location
No unset set Download to specified path
No false unset Error
No false set Use binary (absolute path)
No true unset Download to default location
No true set Download to specified path
Yes unset unset Use binary (from PATH)
Yes unset set Download to specified path
Yes false unset Use binary (from PATH)
Yes false set Use binary (specified path)
Yes true unset Download to default location
Yes true set Download to specified path

@pavelzw
Copy link
Member

pavelzw commented Dec 15, 2023

micromamba-binary-path always causes download = false and fails if no binary file exists at this location.

this would be a breaking change. Also I don't know if we really want to have this behavior. In some other self-hosted scenarios (non-ephermal), you might want to explicitly alter the path to ${{ runner.temp }} and download the binary there (see readme)

@pavelzw
Copy link
Member

pavelzw commented Dec 15, 2023

I like the table, how about we also put it in the readme?

@pavelzw
Copy link
Member

pavelzw commented Dec 15, 2023

The table doesn't cover what's happening when download-micromamba is unspecified.

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 15, 2023

I extended the table above and in the README

Copy link
Member

@pavelzw pavelzw left a comment

Choose a reason for hiding this comment

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

This behavior is fine with me 👍🏻 Can we get some tests (using a matrix maybe)?

README.md Outdated Show resolved Hide resolved
@jonashaag
Copy link
Collaborator

If you need a table to explain the interaction between the options, maybe there is too much interaction going on. Is there a way to simplify things?

Co-authored-by: Pavel Zwerschke <[email protected]>
@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 15, 2023

If you need a table to explain the interaction between the options, maybe there is too much interaction going on. Is there a way to simplify things?

I would want the following use-cases / options for users:

  • Use Micromamba from PATH if it exists and no special option is set
  • Download Micromamba if it is not on PATH and no special option is set
  • Be able to set the download-location to a custom path
  • Be able to use a pre-installed binary from a custom path
  • Be able to ignore the micromamba from PATH and force a redownload

I can't think of any real simpler solutions that allow all use cases without introducing breaking changes.
I also think every row in the table matches the expected behaviour.

Do you have ideas how we could simplify it?

@jonashaag
Copy link
Collaborator

jonashaag commented Dec 15, 2023

IIUC this is the only new feature of this PR:

Use Micromamba from PATH if it exists and no special option is set

And the matrix without this PR is

Micromamba on $PATH download-micromamba micromamba-binary-path Behavior
X false unset Use binary (from PATH, error if missing)
X false set Use binary (absolute path)
X true/unset unset Download to default location
X true/unset set Download to specified path

Simplified, the old cases are

  • Download (to default or custom location): (Download not false)
  • Use existing binary (from default or custom location): Inverse of that, one error case.

With the new feature, the matrix gains 6 new cases.

Simplified, the new cases are

  • Download (to default or custom location): (Download not false AND NOT (Micromamba on path and binary path unset))
  • Use existing binary (from default or custom location): Inverse of that, minus one error case.

Not easy to reason about. I think it's a very high cost just for being able to remove these lines from the caller:

with:
  micromamba-binary-path: micromamba
  download-micromamba: false

Or are trying to solve situations here where we don't know whether Micromamba is in PATH, ie. we can't hardcode these two lines?

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 15, 2023

Or are trying to solve situations here where we don't know whether Micromamba is in PATH, ie. we can't hardcode these two lines?

No, my intention for this PR is to be able to remove those lines.

I am using GitHub actions with self-hosted runners in an environment with limited network access. I use several composite actions that use setup-micromamba themselves. Without this PR, each of these actions would have to re-export an "offline" parameter in order to be used in this environment. With this PR, everything would work out-of-the-box, both for runners with pre-installed Micromamba and for runners without Micromamba.

Also, I don't think "Micromamba on path and binary path unset" is that costly for this feature compared to the alternative of having to customize most of the downstream actions and probably the behavior most users would want, after they installed Micromamba into their runner.

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Jan 11, 2024

I have added a test to ensure that if download-micromamba: true the pre-installed version is ignored and the pre-installed micromamba is used by default.

During this i noticed that setup-micromamba tries to create the micromamba-shell right next to the micromamba binary.

This should probably be changed to a tmp path, as micromamba might be installed into e.g. /usr/local/bin

.github/workflows/test-download.yml Show resolved Hide resolved
.github/workflows/test-download.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/options.ts Show resolved Hide resolved
src/options.ts Outdated Show resolved Hide resolved
src/post.ts Outdated Show resolved Hide resolved
@pavelzw pavelzw changed the title Update micromamba installation process Update micromamba installation process, refactor options Jan 23, 2024
@pavelzw pavelzw merged commit 8767fb7 into mamba-org:main Jan 23, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants