Skip to content

add: abstraction to remove appwrapper visiblilty from NB console. #380

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 1 commit into from
Nov 13, 2023

Conversation

VanillaSpoon
Copy link
Contributor

@VanillaSpoon VanillaSpoon commented Oct 11, 2023

Issue link

closes #365

What changes have been made

To further abstract the appwrapper from a users perspective, they appwrapper generation has been moved to a hidden folder ~/.codeflare/appwrapper/<appwrapper.yaml>.

Verification steps

poetry build
Install the new .whl for the sdk in a jupyter notebook.
Open one of the quick-start demos: git clone https://github.com/project-codeflare/codeflare-sdk

Follow the process as usual.

After your clusterconfiguration ensure this is logged:
Written to: /opt/app-root/src/.codeflare/appwrapper/<appwrapper.yaml>

The yaml will no longer appear in the listed files.

From the notebook terminal, you can cd to ~/.codeflare/appwrapper/ and then ls. This will list the generated .yaml. Feel free to inspect it with vi <appwrapper-name.yaml> and ensure the appwrapper name and information is correct.

Continue with cluster.up() and ensure the appwrapper is applied and raycluster is deployed as expected, with the correct cluster info in the kuberay-operator pod logs.

Checks

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

@Fiona-Waters
Copy link
Contributor

Ran through verification steps and can confirm that appwrapper yaml file is written to .codeflare directory. Ran a demo notebook and all works as expected.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2023
Copy link
Collaborator

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

One thing to note, I think this should probably be in an absolute path, ~/.codeflare/... (or expand ~ accordingly to user home dir), rather than a relative path based on current working directory. This way we match the kubernetes pattern of ~/.kube/...

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023
@VanillaSpoon
Copy link
Contributor Author

Thanks for the feedback @Maxusmusti
The requested changes have been applied in 0ef1256, altering the path to ~/.codeflare/appwrapper/.

Within jupyterhub this will be /opt/app-root/src/.codeflare/appwrapper/<appwrapper.yaml> accessible as in the pr description but within ~/.codeflare/appwrapper from the jupyterhub terminal.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2023
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.

Tested this and it works as expected
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023
@KPostOffice
Copy link
Collaborator

KPostOffice commented Oct 30, 2023

Sorry for the late review, but if we're trying to abstract the creation of the appwrapper file from the user, what is the purpose of creating a yaml file at all? Why not just generate the raw text into memory at cluster creation time and optionally allow the file to be generated for debugging purposes and testing?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2023
@Maxusmusti
Copy link
Collaborator

@KPostOffice Adding the slack comment here as well:

It's something @MichaelClifford, @dimakis, and I have previously discussed, essentially providing the actual yamls themselves is still useful for a lot of users that may want to tweak them with options the SDK doesn't currently support, check them before submitting to their cluster, re-use them, etc.

At the same time, using relative paths was a bit messy, and there were users (mainly BU), who didn't want the AppWrappers generated in their current working dir. So to both organize all codeflare-related assets, as well as to clean/hide implementation details for those who don't care while keeping them accessible for those who do, we ended up deciding on copying the kubernetes ~/.kube/ pattern with ~/.codeflare/ dir for asset management and accessibility

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2023
@VanillaSpoon
Copy link
Contributor Author

Thanks for raising the discussion on this @KPostOffice, and thank you @Maxusmusti for providing additional information :)

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 Nov 13, 2023
Copy link
Contributor

openshift-ci bot commented Nov 13, 2023

[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 Nov 13, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit c212bd8 into project-codeflare:main Nov 13, 2023
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.

SDK saves Appwrapper yaml to ~/.codeflare/appwrapper/<aw.yml>
6 participants