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.

Sign up for GitHub

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

Jump to bottom

mv demo nbs into src folder and add fn for cloning to current dir #605

Merged
merged 1 commit into from
Jul 26, 2024
Merged

mv demo nbs into src folder and add fn for cloning to current dir #605

merged 1 commit into from
Jul 26, 2024

Conversation

KPostOffice
Copy link
Collaborator

Issue link

https://issues.redhat.com/browse/RHOAIENG-10266

What changes have been made

Be not afraid of the many files being changed, they are all just being moved from the root to be inside the sdk src dir

Verification steps

  1. build package via poetry build
  2. create venv and activate it
  3. install built wheel by doing pip install /dist/codeflare_sdk-0.0.0.dev0-py3-none-any.whl`
  4. activate python shell
  5. run import codeflare_sdk
  6. run codeflare_sdk.clone_demo_nbs()
  7. verify that demo-notebooks dir is created with all demo nbs present

Checks

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

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2024
@KPostOffice KPostOffice force-pushed the rhoaieng-10266-demo-nbs-in-build branch 2 times, most recently from e2fa5d6 to f3b8f5d Compare July 24, 2024 15:10
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2024
Ygnas
Ygnas approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@Ygnas Ygnas left a comment

Choose a reason for hiding this comment

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

Works as expected.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2024
Bobbins228
Bobbins228 approved these changes Jul 25, 2024
View reviewed changes
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.

/approve
Works as expected awesome work on this Kevin!

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024
@Bobbins228 Bobbins228 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024
@Bobbins228
Copy link
Contributor

@KPostOffice had to remove approval just had a thought. We should document this in our README

astefanutti
astefanutti reviewed Jul 25, 2024
View reviewed changes
Copy link
Contributor

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

Would a symlink work?

@KPostOffice
Copy link
Collaborator Author

Would a symlink work?

@astefanutti I think copies make more sense that way users always have access to the original demo notebooks in case they adjust it and make some changes that cause it to break

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024
@KPostOffice KPostOffice force-pushed the rhoaieng-10266-demo-nbs-in-build branch from f3b8f5d to 00704d7 Compare July 25, 2024 18:45
@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Jul 25, 2024
@KPostOffice KPostOffice force-pushed the rhoaieng-10266-demo-nbs-in-build branch from 00704d7 to 6be9d23 Compare July 25, 2024 18:45
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024
@astefanutti
Copy link
Contributor

Would a symlink work?

@astefanutti I think copies make more sense that way users always have access to the original demo notebooks in case they adjust it and make some changes that cause it to break

I'm talking about a symlink from the codeflare-sdk in the codebase / repository, not when users call the function to copy the demo notebooks.

@Bobbins228 Bobbins228 mentioned this pull request Jul 26, 2024
Merged
4 tasks
@KPostOffice KPostOffice force-pushed the rhoaieng-10266-demo-nbs-in-build branch from 6be9d23 to f5f449f Compare July 26, 2024 12:13
Bobbins228
Bobbins228 reviewed Jul 26, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
ChughShilpa
ChughShilpa reviewed Jul 26, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@KPostOffice KPostOffice force-pushed the rhoaieng-10266-demo-nbs-in-build branch from f5f449f to 38f2a75 Compare July 26, 2024 12:27
Bobbins228
Bobbins228 reviewed Jul 26, 2024
View reviewed changes
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

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

/lgtm

Bobbins228
Bobbins228 approved these changes Jul 26, 2024
View reviewed changes
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.

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228, Ygnas

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-merge-bot openshift-merge-bot bot merged commit 62ce155 into project-codeflare:main Jul 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChughShilpa ChughShilpa ChughShilpa left review comments

@astefanutti astefanutti astefanutti left review comments

@Bobbins228 Bobbins228 Bobbins228 approved these changes

@Ygnas Ygnas Ygnas approved these changes

@MichaelClifford MichaelClifford Awaiting requested review from MichaelClifford

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

@astefanutti astefanutti

@Ygnas Ygnas

@Bobbins228 Bobbins228

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KPostOffice @Bobbins228 @astefanutti @Ygnas @ChughShilpa @openshift-merge-robot