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 command to palette to open Runtime Images UI #835

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

ajbozarth
Copy link
Member

As part of the previous work on Metadata Widget we added a generic
command to open a UI for any metadata namespace/schema

This PR adds a command call to open the ui for runtime images to
the palette.

Fixes #829

Screen Shot 2020-08-06 at 4 46 26 PM

Screen Shot 2020-08-06 at 4 46 13 PM

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

As part of the previous work on Metadata Widget we added a generic
command to open a UI for any metadata namespace/schema

This PR adds a command call to open the ui for runtime images to
the palette.

Fixes elyra-ai#829
@ajbozarth ajbozarth self-assigned this Aug 7, 2020
@ajbozarth
Copy link
Member Author

@ptitzler does this handle what you wanted in #829 ?

@ptitzler
Copy link
Member

ptitzler commented Aug 7, 2020

@ptitzler does this handle what you wanted in #829 ?

Please see new comment in that issue about where/how it is exposed.

A few additional thoughts that are mostly specific to this editor instance (but some of the points raised are of generic nature and should be reviewed):

Summary view

image

It'd be great if the information could be displayed in a formatted way (which might include a link to the image in the public registry), not just as a JSON dump. This would be similar to how we currently display runtime configuration metadata.

Add/edit view

image

Input validation

Having tried adding a runtime image using the CLI I've noticed that no validation is performed and any bogus name is accepted. While there might be valid reasons why an image name cannot be resolved, there's also a chance that the user accidentally mistyped the name (or tag). As is, this problem would (I guess, need to test this to confirm) only surface when pipeline execution fails. Since that is pretty late in the workflow it'd be nice if the editor (or backend) could try to validate that the specified image can be accessed when a new runtime image is added or an image is edited. This way a user would know right away that there might be a problem and has the option to correct the issue or take the risk that things will fail.

Dangling references

If I'm not mistaken this editor is the first of its kind that might cause dangling references. For example:

  • create a custom runtime image
  • reference this runtime image in a notebook pipeline
  • delete the runtime image; the notebook pipeline is now invalid

We need to think about the implications:

  • Should the editor display a warning that this runtime image might be used somewhere and that something might break as a result? (Elyra won't know where it is used.)
  • Do the consumers of this piece of metadata already try to catch this error condition?

Changing image references

The metadata currently comprises of a [display] name and the image name. It appears that the pipeline definition only includes the Docker image name. Now that users can explicitly define their own images (or change the name-> image mapping) a user might observe unexpected behavior:

  • create a custom runtime image
  • reference this runtime image in a notebook pipeline (notebook X uses runtime Y, Y mapping to docker image "y")
  • change the metadata mapping from Y-> "y" to Y -> "z"
    A user might expect that the mapping in the pipeline has also changed, but that is not the case. To set expectation we probably need to issue a warning if a mapping is changed indicating that existing "references" are not updated.

@kevin-bates
Copy link
Member

kevin-bates commented Aug 7, 2020

Regarding dangling references and changing image references, these are somewhat conflicting solutions. If you solve one, you have the other and vice versa. I had noticed the same thing when I discovered that we only store the image name in the pipeline node, not the metadata instance reference. When bringing this up, we decided to err on the side of reproducibility (and address potential dangling references) rather than resolve the changing image references. I'm not sure which is better. The other reason we have (currently) gone with the current approach is because that's what we're doing already. 😄

I do agree it would be nice to search through pipelines to see where a particular image or metadata instance might be referenced, however.

Regarding input validation I also agree and is another side-effect of schema-driven approaches and yet another example of needing meta-behaviors or custom metadata classes so that property-specific actions can be performed.

@ajbozarth
Copy link
Member Author

@ptitzler in response to both your comment here and on Issue #829 (comment)

I don't currently have the bandwidth this sprint to address your concerns (I opened this PR as a "quick fix" while working on another issue). It does seem like most of your concerns on this though are of a "follow-up" level, that is that this PR does the job, but it should do it better. As such I will leave this as is for now, if other reviews are good we can merge this as part of 1.1 and address your concerns as a follow up PR in 1.2 next sprint when I have bandwidth again.

@ptitzler
Copy link
Member

ptitzler commented Aug 8, 2020

are of a "follow-up" level

Yes, that's fine. I just wanted to get the ball rolling. I will transfer the topics to separate issues where they can be discussed in more detail.

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

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

Tried this locally and it's working - I agree with the comments above but for a first pass this should work. LGTM

@karlaspuldaro
Copy link
Member

Works great. LGTM :)

@vabarbosa
Copy link
Contributor

vabarbosa commented Aug 13, 2020

how often do people actually look through or use the Commands palette? would it be worthwhile adding a corresponding tile in the Elyra section of the Launcher where it would more noticeable?

@ajbozarth
Copy link
Member Author

how often do people actually look through or use the Commands palette? would it be worthwhile adding a corresponding tile in the Elyra section of the Launcher where it would more noticeable?

AFAIK the Launcher is essentially a pretty UI for File > New and is designed for opening new doc files, not widgets in general

@vabarbosa
Copy link
Contributor

how often do people actually look through or use the Commands palette? would it be worthwhile adding a corresponding tile in the Elyra section of the Launcher where it would more noticeable?

AFAIK the Launcher is essentially a pretty UI for File > New and is designed for opening new doc files, not widgets in general

there is already "Show Contextual Help" tile (in the Other section). i don't believe that is a File > New action.

@ajbozarth
Copy link
Member Author

@vabarbosa I didn't realize that.

I brought up the point in this mornings dev meeting and after a short discussion we came to this point:
This functionality is too "low level" and minor to put on the very high visibility Launcher. Though adding things to the Launcher might make sense in the future, this is not the best case of one. There's also the fact that as of now there's no way to close a sidebar tab, meaning if a user opens the UI the only way to close it is to refresh the page.

The short of it all is that this is a very minor feature and wont be used much, the important part is the ability to use it at all. As such a command on the palette is the most straight forward location for it. This also allows for the addition of more such commands when future metadata instance are added that don't warrant a hight profile UI (like code snippets does)

Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

LGTM

@lresende lresende merged commit 2429396 into elyra-ai:master Aug 14, 2020
@ajbozarth ajbozarth deleted the images-ui branch August 18, 2020 01:11
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.

Pipelines: As a user I want to define/use a custom runtime/Docker image to run notebooks in using the UI
7 participants