Skip to content

ipywidgets UI create delete #668

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

Merged

Conversation

Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Sep 16, 2024

Issue link

Closes: RHOAIENG-12523

What changes have been made

  • Added ipywidgets to act as buttons for the cluster.up() and cluster.down() methods
  • Added print statements to confirm cluster creation/deletion
  • Added unit tests for buttons
  • Added wait_ready Check Box

image
Happy days :)

Verification steps

Setup

Notebook server ODH/RHOAI/Local

  • Clone this repository with git clone https://github.com/project-codeflare/codeflare-sdk.git
  • Checkout this PR's branch
  • Run poetry build - install if needed (pip install poetry)
  • Run pip install --force-reinstall dist/codeflare_sdk-0.0.0.dev0-py3-none-any.whl
  • Restart your notebook kernel

Testing

  • Run through any example notebook
  • On the ClusterConfiguration step the ui buttons for cluster up/down should appear
  • Verify the buttons work as expected
  • Run the ClusterConfiguration as a script and the buttons should not appear.
  • Run in a RHOAI notebook and in a VSCode local notebook and verify buttons are created

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@Bobbins228 Bobbins228 changed the title UI create delete ipywidgets UI create delete Sep 16, 2024
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Looks great, I've left a couple of comments

Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Great work! Just one small nitpick

Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Should we think of moving UI related items into their own file? - And keep strictly functionality in the cluster.py file?

@Bobbins228
Copy link
Contributor Author

@ChristianZaccaria I had a similar thought when reading through the design doc.
I think we should go for it as to not clog the cluster.py file with the widgets.

@Bobbins228
Copy link
Contributor Author

@ChristianZaccaria On second thought that may not be the best for this particular widget.
It requires the Cluster Object for the up() and down() methods so it is seemingly the only widget that should live in cluster.py.

@ChristianZaccaria
Copy link
Collaborator

ChristianZaccaria commented Sep 17, 2024

@Bobbins228 perhaps the UI itself (only the buttons for example) can be separated into its own file, and the functionality (up() and down()) remain in the cluster.py file.

@Bobbins228 Bobbins228 force-pushed the ui-create-delete branch 4 times, most recently from a4b08ea to db8eeac Compare September 17, 2024 12:11
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

One change, other than that I think it's g2g

Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Just tested this all out and it works great! I have the smallest change ever then ready for merge I think

Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2024
Copy link
Contributor

openshift-ci bot commented Sep 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KPostOffice

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2024
@KPostOffice
Copy link
Collaborator

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2024
Copy link
Contributor

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 9794a0f into project-codeflare:main Sep 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants