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

Lockfile upload #401

Merged
merged 29 commits into from
Aug 10, 2024
Merged

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented May 15, 2024

Closes #399.

Companion backend PR: conda-incubator/conda-store#772

Description

This PR takes over #395.

PR 395 implemented "create environment from lockfile" using a code editor. But because hand-editing lockfiles is error prone, we decided that the UI should be implemented as a file upload instead.

This pull request:

  • Reuses the code from Add ability to create environment from lockfile #395 that loads environment info if the environment was created with a lockfile
  • Adds dependency, mui-file-dropzone, to quickly implement the file drop upload
  • Alert dialog accepts text for the primary button, instead of always displaying Cancel | Delete
  • For create and edit environment pages, add UI to switch from specification to lockfile upload
  • For view environment page, add UI to show packages installed by the lockfile (as well as link to download the lockfile)

Screenshots

New environment form showing the "Switch to Conda lockfile upload" button in the header of the specification form field:

Dialog that warns you when you switch from Specification to Lockfile that you may lose your work:

New environment form showing the new lockfile upload UI with the "Switch Specification" button in the header of the file upload form field, and a small print note below the file drop area specifying that only Conda lockfile format is supported:

The view environment page for a build that was built from a lockfile, showing both a download link and the packages installed:

The YAML editor now has a note saying that only the Conda environment.yml format is supported:

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)?

@gabalafou
Copy link
Contributor Author

Creating this as a draft PR to solicit feedback. Couple questions:

  • Should the user be able to use the lockfile upload on the Edit Environment form in addition to the Create Environment form?
  • The form needs some cosmetic improvements, but apart from the cosmetics, is this the direction we want this form to go in? Or do we have alternative ideas for how to structure it?

@dharhas
Copy link
Member

dharhas commented May 15, 2024

So if I upload a lockfile, what happens to the 'Specification' Tab. Does it get disabled?

@gabalafou
Copy link
Contributor Author

So if I upload a lockfile, what happens to the 'Specification' Tab. Does it get disabled?

Not at the moment. I think that's worth implementing if we stick with this direction because otherwise I think it can create the misleading impression that the two tabs are synced with each other in the same way that the GUI-YAML toggle is.

@trallard trallard requested a review from smeragoel May 28, 2024 17:43
@gabalafou
Copy link
Contributor Author

Screen.Recording.2024-06-12.at.10.40.50.mov

@smeragoel @kcpevey, what do we think about the implementation shown in this video?

@kcpevey
Copy link
Contributor

kcpevey commented Jun 12, 2024

@gabalafou I really like it!!

I'm wondering about what it looks like AFTER you upload the lockfile, or rather, the view of an existing environment built by lock file. Is that what test_env1269 is? In that case, what does it look like when you go Edit (via spec) -> YAML? Are the requested/installed packages mocked here or is that already functional once build via lockfile?

@gabalafou
Copy link
Contributor Author

Great! Good question. I haven't coded this up yet, but I was thinking that when you load an environment that was last built from a lockfile, you will see that same box that says "Lockfile" but instead of "upload" it will say "download". Then, if you click the "edit" button that section will become upload again. Am I making sense or do we need a visual?

@kcpevey
Copy link
Contributor

kcpevey commented Jun 13, 2024

I followed you, and I think what you described are pieces that need to be included. I'd also like to see an uneditable view of the lockfile that is currently loaded.

For example, in a specification-based environment, when I view an existing environemnt I see requested and installed deps. When I view an existing lockfile environment (as far as I can tell), we won't have any access to requested or installed deps and the best "view" we can get is the lockfile itself.

@gabalafou
Copy link
Contributor Author

Great feedback. Let me code up quickly what all I think I'm hearing, and we'll iterate from there.

@dharhas
Copy link
Member

dharhas commented Jun 24, 2024

Do we need to add descriptive text to indicate that these are "conda env.yaml" files and "conda lockfiles". i.e. to avoid folks trying to upload poetry lock files etc?

- On VIEW page for environment built from lockfile, show link to lockfile
- In specification mode of EDIT page for environment built from lockfile,
  show clean slate
@gabalafou

This comment was marked as outdated.

@gabalafou
Copy link
Contributor Author

Visual tour of latest changes

View environment - lockfile

This screenshot shows the "view environment" page for an environment whose active build was created from a lockfile upload. Of note:

  • User does not have to actually view the .conda-lock.yml file to see the dependencies that were installed in this build. They are listed in a table. The channels that were used are also listed.
  • Form section now says "Conda Lockfile" to reenforce the idea that this is a Conda Lockfile and not some other format

From this view, if you click the edit button, you get taken to...

Edit environment - lockfile mode

Not much new here to note except that the upload section of the form is now titled "Conda Lockfile Upload," again to reenforce the idea that it is for Conda lockfiles and not some other format.

From this view, if you click the "Conda Lockfile Upload" heading, it expands to show...

Note about Conda lockfile format

We currently only support the Conda lockfile format. Other lockfile formats (such as Poetry) are not supported.

From this view, if you click "Switch to Specification", you get taken to...

Switch to Specification warning

From this view, if you click "Continue", you get taken to...

Edit environment - GUI specification mode

Here I've implemented the request that when going into specification mode from a lockfile build, the user starts with a clean slate. None of the dependencies in the lockfile are shown in this view. The user is in fact starting over. If they click save here, the environment is rebuilt from the specification shown in the GUI. The lockfile is completely forgotten.

(Oh hmm, I just noticed while making this that "channels" is not reset. I think it's carrying the value of all the channels found in the lockfile. I should probably reset that.)

From this view, if you click the toggle from GUI to YAML, you get taken to...

Edit environment - YAML specification mode

I added a details/summary above the YAML with the summary line "Conda environment.yml"

If you click the summary line, it expands to show...

Note about Conda environment.yml format

We currently only support the Conda environment.yml format. Other environment specification file formats are not supported.

The end.

@kcpevey
Copy link
Contributor

kcpevey commented Jul 16, 2024

The end.

Standing Ovation!!

This design is wonderful! Also kudos for presenting all that info in an understandable fashion :)

The ONLY thing that I'll give minor pushback on is the details/summary under conda environment.yml and conda lockfile upload. It feels like a lot of UI for just that quick comment. Could we instead add a little info button circle and instead of an "i" in the circle, its a "!" (not sure if that's a thing, but I'm just thinking of drawing a little more attention to it). Honestly, this is a nitpick and I'm flexible on this point.

Other commentary:

User does not have to actually view the .conda-lock.yml file to see the dependencies that were installed in this build. They are listed in a table. The channels that were used are also listed.

Definitely the right choice.

Here I've implemented the request that when going into specification mode from a lockfile build, the user starts with a clean slate.

I agree.

@smeragoel
Copy link

Thanks for presenting this @gabalafou, and for wrangling all the limitations and figuring out this workflow!

Could we instead add a little info button circle and instead of an "i" in the circle, its a "!" (not sure if that's a thing, but I'm just thinking of drawing a little more attention to it).

I agree with this as well. I like the idea of presenting more information to the user, and I think we can do it via a tooltip or something similar as suggested by Kim.

- pull info out from details/summary
- restyle cancel/continue buttons on alert dialog
- restyle "switch to" buttons and move up
@gabalafou gabalafou marked this pull request as ready for review July 23, 2024 17:37
@gabalafou
Copy link
Contributor Author

gabalafou commented Jul 23, 2024

So I started trying to implement "more icon" as suggested, but tooltips like these have accessibility challenges and I would rather that we avoid them if possible.

What do we think about this alternative, in which the info is displayed below both form controls (the yaml editor and the drag-and-drop box) in small font:

@kcpevey
Copy link
Contributor

kcpevey commented Jul 23, 2024

That alternative is a great! Thanks for keeping accessibility concerns in mind :)

@trallard trallard added the type: enhancement 💅🏼 New feature or request label Aug 6, 2024
@smeragoel
Copy link

Sorry for the delay in review. I think your solution is great @gabalafou, and the design is approved!

@gabalafou gabalafou mentioned this pull request Aug 8, 2024
@gabalafou
Copy link
Contributor Author

I just applied #411 to this PR to see if it fixes CI

@gabalafou
Copy link
Contributor Author

Okay, all the checks passed so I think it was two things previously causing it to fail:

  1. Version mismatch between my local Yarn (v1) and the CI Yarn (which I think is v4)
  2. docker-compose versus docker compose (Fix Docker Compose for GitHub Actions #411)

Before merging this PR, we need to merge #411, and then sync this PR with the main branch.

@gabalafou
Copy link
Contributor Author

gabalafou commented Aug 9, 2024

With regards to the first problem, I've tried to make the Yarn version more explicit in #412.

@peytondmurray
Copy link
Contributor

Looks like this is passing now. I'll merge it!

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

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

Thanks for this, it looks fantastic!

@@ -1,5 +1,3 @@
version: "3.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,2 @@
// TODO: define lockfile type better
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth making an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, done! #413

@peytondmurray peytondmurray merged commit 0885e02 into conda-incubator:main Aug 10, 2024
4 checks passed
@gabalafou gabalafou deleted the lockfile-upload branch August 19, 2024 09:47
@gabalafou
Copy link
Contributor Author

@peytondmurray thanks!

gabalafou added a commit to gabalafou/conda-store-ui that referenced this pull request Aug 19, 2024
* Support creating lockfile envs

* Remove redundant import

* Update state

* Remove workarounds that are no longer needed

* Handle `is_lockfile` in test

* Initial edit spec support

* Send updated spec to server

* Get channels from lockfile

* Get dependencies from lockfile

* gabs edits

* Add warning about no syncing between lockfile and GUI

* Revert unneeded changes

* Working proof of concept: lockfile upload

* Switch to lockfile/specification

* Show dependencies and channels for lockfile-built envs

* reset SpecificationEdit when switching builds

* Kim's suggestions

- On VIEW page for environment built from lockfile, show link to lockfile
- In specification mode of EDIT page for environment built from lockfile,
  show clean slate

* Show installed dependencies and channels on view environment built from lockfile

* lint

* yarn install

* yarn install (with correct version of yarn this time)

* note about file formats

* lint

* Three changes:

- pull info out from details/summary
- restyle cancel/continue buttons on alert dialog
- restyle "switch to" buttons and move up

* update yarn.lock

* Fix Docker Compose for GitHub Actions

---------

Co-authored-by: Nikita Karetnikov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement 💅🏼 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] - Add ability to build env from lockfile
7 participants