Skip to content

Add UI table with Cluster specs and action buttons #681

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
merged 14 commits into from
Sep 27, 2024

Conversation

ChristianZaccaria
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria commented Sep 23, 2024

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-12526

What changes have been made

image

  • Added UI table that will display existing clusters with their specs, and handle user interactions which include:
    • Selecting a cluster.
    • Deleting the selected cluster.
    • Opening in a new tab the Ray Dashboard of the selected cluster.
    • Opening in a new tab the Ray Dashboard Job view of the selected cluster.
  • Formatted table to merge req~lim resources into one column.
  • Formatted status field to change text color and add an icon based on the cluster status.
  • Added unit tests.
  • Added outputs.
  • Updated Ray model with cpu requests and cpu limits.

TODO:

  • Add view_clusters to guided-notebooks by default? - Given Cluster Up and Cluster Down widgets will appear by default, I think it's a good idea to immediately encourage the user to use (and be aware) of this feature. We don't need to trigger the buttons for test-guided-notebooks tests as it will be covered by ui-notebooks-test tests. Only the expected outputs would have to be updated.
  • Enhance unit tests.
  • Add to UI tests
  • Implement TODO left in widgets.py file. [ ✔️ ]

Verification steps

Install the codeflare-sdk and import:

from codeflare_sdk import view_clusters

There are several items to test which include:

  • Create several clusters.
  • Select a cluster.
  • Click on each button.
  • Switch clusters and click on each button.
  • Verify spec displayed.
  • Verify output.
  • Repeat with AppWrappers.

Checks

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

@ChristianZaccaria ChristianZaccaria changed the title Table widgets Add UI table with Cluster specs and action buttons Sep 23, 2024
@ChristianZaccaria ChristianZaccaria added the test-ui-notebooks Run PR check to verify UI notebooks label Sep 25, 2024
@Ygnas Ygnas added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 25, 2024
@ChristianZaccaria ChristianZaccaria added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 25, 2024
@ChristianZaccaria ChristianZaccaria added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 25, 2024
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Just a few nits looks good other than that. I will physically test this now but I just have another annoying nit on top of all of this.

Can you add head comments for all of the new functions of the wdgets.py file? Similar to what you have done here.

@Bobbins228
Copy link
Contributor

@ChristianZaccaria can you run the pre-commit run --all-files command on your next commit :)
For future ease you can run pre-commit install within the project and every time you make a commit the pre-commit hook will run stopping you from commiting if your files are not up to the format.

@ChristianZaccaria ChristianZaccaria force-pushed the table-widgets branch 5 times, most recently from c21efee to 4070c59 Compare September 26, 2024 16:34
@Bobbins228
Copy link
Contributor

@ChristianZaccaria The ray image is fixed now so E2E should pass. Can you rebase?

@ChristianZaccaria ChristianZaccaria added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 27, 2024
@ChristianZaccaria
Copy link
Collaborator Author

ChristianZaccaria commented Sep 27, 2024

Remaining enhancements will be made in the scope of the Jira "Refactor ipywidgets into a separate module" from the Refactor CodeFlare-SDK epic.

This includes:

  • Enhance unit tests to increase coverage.
  • Convert widgets to its own class.
  • Find a way of leveraging cluster.down to re-use code.

Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

openshift-ci bot commented Sep 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228

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 27, 2024
@ChristianZaccaria ChristianZaccaria added test-ui-notebooks Run PR check to verify UI notebooks and removed test-ui-notebooks Run PR check to verify UI notebooks labels Sep 27, 2024
@Bobbins228 Bobbins228 merged commit c2eaa15 into project-codeflare:main Sep 27, 2024
16 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. test-ui-notebooks Run PR check to verify UI notebooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants