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 ability to create environment from lockfile #395

Closed
wants to merge 10 commits into from

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Apr 29, 2024

This is a companion PR for conda-incubator/conda-store#772.

Description

This pull request: adds a new dropdown in the YAML window, which allows creating an env from a lockfile.

The supported lockfile type is https://conda.github.io/conda-lock/output/#unified-lockfile, which is also generated by conda-store as the lockfile build artifact.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

Where to get a lockfile for testing:

  • create a standard conda-store environment
  • copy the generated lockfile from the build artifacts
  • create an env with this lockfile by selecting the lockfile specification type and pasting the copied lockfile.

Manual tests:

  • can create an environment using YAML -> lockfile specification type, deps and channels are updated on the GUI page on save
  • can edit lockfile environment, same
  • can edit a standard env and use lockfile type for the new revision
  • can edit a lockfile env and use specification type for the new revision
  • expected to not work: GUI is not updated without saving the env
  • expected to not work: lockfile is created from a GUI alone (not enough information in the GUI to create a lockfile entry)
  • expected to not work: no data is migrated when switching between specification types without saving

Future work:

  • After this is merged, docs should be added to the conda-store repo describing the lockfile specification type. The supported lockfile type is https://conda.github.io/conda-lock/output/#unified-lockfile
  • I had no time to create auto tests, some of the manual tests could be converted to auto tests.

@@ -6,4 +6,5 @@ export type CondaSpecification = {
dependencies: (string | CondaSpecificationPip)[];
variables: Record<string, string>;
prefix?: string | null;
lockfile?: any | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to rely on any particular format here since it might change upstream.

spec?.lockfile?.metadata?.channels?.map(
(channel: any) => channel?.url
) ??
[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and similar code for deps is what populates GUI channels and dependencies lists on env save.

metadata: {},
package: []
});
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to hint to the user which lockfile format is expected. Similar idea to formatCode above.

@@ -0,0 +1,41 @@
import { createSlice } from "@reduxjs/toolkit";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is the same idea as src/features/environmentVariables/environmentVariablesSlice.ts.

spec?.lockfile?.package
?.filter((p: any) => p?.manager === "conda")
?.map((p: any) => `${p?.name}==${p?.version}`) ??
[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only list conda packages and ignore pip because conda-store does the same for ordinary specs as well.

displayEmpty
>
<MenuItem value="specification">specification</MenuItem>
<MenuItem value="lockfile">unified lockfile</MenuItem>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MenuItem values here match those we show in the admin interface (see the companion conda-store PR).

@nkaretnikov nkaretnikov marked this pull request as ready for review May 1, 2024 13:36
@nkaretnikov
Copy link
Contributor Author

@gabalafou @peytondmurray PTAL. I'm currently on PTO, feel free to take over if this requires changes.

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

I pushed a few small changes.

I tested locally by copy-pasting lockfile from one environment to create another.

I feel that we should add a note to the UI that explains that GUI-Lockfile syncing has not been implemented. Otherwise I think an end user will find the app behavior confusing. I'll add that in a subsequent commit and then I think this should be good to go.

@gabalafou
Copy link
Contributor

I just chatted with @trallard about this. I'm not sure we can move forward with this PR. This PR implements a UI that suggests to the end user that they can manually edit a lockfile, which can lead to very unintuitive behavior. For example (one we tested), if a user updates the version of a package in the lockfile, then clicks save, the version does not update because the backend (apparently) uses the url property and not the version string.

It seems that the correct implementation here should be a file upload button for the lockfile, not exposing it in the code editor.

@gabalafou
Copy link
Contributor

gabalafou commented May 7, 2024

Based on sprint planning meeting today, I'm closing this PR without merging because I will re-implement the UX for "create environment from a lockfile" as a file upload instead.

@gabalafou gabalafou closed this May 7, 2024
@gabalafou gabalafou mentioned this pull request May 15, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

2 participants