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

Add --force-platform to conda-lock install command. #753

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jezdez
Copy link
Member

@jezdez jezdez commented Nov 18, 2024

Description

This selects a specific platform for installation instead of the current, native platform. Should be similar to conda/conda#11794, kinda.

This selects a specific platform for installation instead of the current platform.
@jezdez jezdez requested a review from a team as a code owner November 18, 2024 08:55
Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 60792e1
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/673c67599b866300088c8141
😎 Deploy Preview https://deploy-preview-753--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jezdez jezdez changed the title Add --platform to conda-lock install command. Add --platform to conda-lock install command. Nov 18, 2024
@maresb
Copy link
Contributor

maresb commented Nov 18, 2024

Thanks @jezdez! This makes sense.

Some initial thoughts:

  1. We should add a test
  2. I'm debating whether calling it the mundane --platform option is a good idea, since the use case is fairly obscure. Probably it's good for consistency, but I think we should at least indicate in the help string that the option is for forcing an override of the native platform, rather than something routine
  3. For internal consistency, how about renaming install(platform=...) to install(force_platform=...)?

@maresb
Copy link
Contributor

maresb commented Nov 18, 2024

Hi @jezdez, unrelated question regarding #752...

Do you know who I'd need to approve this access token request?

image

@jezdez
Copy link
Member Author

jezdez commented Nov 18, 2024

@maresb Yep, this went to the org admins and I just approved it! (more on the PR tomorrow)

@jezdez jezdez changed the title Add --platform to conda-lock install command. Add --force-platform to conda-lock install command. Nov 19, 2024
@jezdez
Copy link
Member Author

jezdez commented Nov 19, 2024

Thanks @jezdez! This makes sense.

Some initial thoughts:

  1. We should add a test

I've added one, reusing one of the lockfiles to mimic installing an Intel macOS package on the macOS ARM64 GitHub runners.

  1. I'm debating whether calling it the mundane --platform option is a good idea, since the use case is fairly obscure. Probably it's good for consistency, but I think we should at least indicate in the help string that the option is for forcing an override of the native platform, rather than something routine

I've updated the description

  1. For internal consistency, how about renaming install(platform=...) to install(force_platform=...)?

That's fine, I've updated both the argument name as well as the CLI option to make sure it's obvious that it's not quite the same as the --platform of the render and lock subcommands.

assert _check_package_installed(
package=package,
prefix=str(prefix),
subdir=platform,
Copy link
Member Author

Choose a reason for hiding this comment

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

It's checking the subdir value of the metadata here

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

This looks really great @jezdez!

We can get this into the upcoming release. I still want to take one more careful look at it before merging.

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.

2 participants