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

Refactoring to Use Poetry #305

Merged
merged 12 commits into from
Sep 14, 2023

Conversation

Fiona-Waters
Copy link
Contributor

@Fiona-Waters Fiona-Waters commented Aug 14, 2023

Issue link

#274

What changes have been made

  • The .gitignore has been updated so that the poetry.lock file is committed to the repo - allowing everyone to use the same versions of dependencies.

  • The main requirements.txt file in the root of the repo has been removed.

  • The readme has been updated to include poetry install rather than using pip

    • Instructions have also been added allowing creation of a requirements.txt file if required
  • The pre-commit Containerfile has been updated to use poetry, the build command will now be run from the root directory to allow access to the pyproject.toml file and all required dependencies.

  • Any installation commands and documentation/readmes have been updated to reflect the changes.

  • We will not remove the requirements.txt files from the demo notebooks. For more info see thread here.

Verification steps

  • First checkout this pr
  • Run poetry install
  • Run poetry build

To verify the building of the precommit image

  • In the root directory run podman build -f .github/build/Containerfile . and ensure that the build completes successfully.

Ensure that unit tests pass

  • In the root directory run the unit tests with poetry run pytest -v tests/unit_test.py

Test demo notebooks

  • Create a cluster
  • Clone the distributed workloads repo
  • Log in to your cluster
  • Install the codeflare stack from the distributed workloads repo by running the following command make all-in-one
  • Once the installation of all components is complete, navigate to the ODH dashboard and to a Jupyter Notebook, and navigate to the terminal.
  • Uninstall the current codeflare-sdk by running pip uninstall codeflare-sdk -y
  • Drag and drop the repo build created by the poetry build command run earlier into the Jupyter notebook and run pip install codeflare_sdk-0.0.0.dev0-py3-none-any.whl
  • Restart the Notebook kernel
  • Run through a couple of demo notebooks and ensure that they run successfully

Checks

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2023
.github/build/Containerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

README.md Outdated Show resolved Hide resolved
@Fiona-Waters
Copy link
Contributor Author

It'd be great to update the SDK e2e test and make sure it passes with Poetry:

https://github.com/project-codeflare/codeflare-operator/blob/6f7d6b1194dd88aa0b2ab08b5b0e1856793883dd/test/e2e/mnist_raycluster_sdk_test.go#L164

Thanks for the review @astefanutti Yes we should definitely do this. Do you think it would be okay if I create a new/follow on ticket for this, or would that need to be completed before merging?

@astefanutti
Copy link
Contributor

It'd be great to update the SDK e2e test and make sure it passes with Poetry:
https://github.com/project-codeflare/codeflare-operator/blob/6f7d6b1194dd88aa0b2ab08b5b0e1856793883dd/test/e2e/mnist_raycluster_sdk_test.go#L164

Thanks for the review @astefanutti Yes we should definitely do this. Do you think it would be okay if I create a new/follow on ticket for this, or would that need to be completed before merging?

@Fiona-Waters creating a follow up ticket to have it done along the next SDK dependency upgrade in the CodeFlare operator sounds the best course of action 👍🏼.

@Fiona-Waters Fiona-Waters marked this pull request as ready for review August 28, 2023 11:49
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2023
@ChristianZaccaria
Copy link
Collaborator

Excellent job on this PR! 👏

To fix the pre-commit checks:

  • Extra white spaces around the Readme file: if using VSCode what I like to do is activate Render Whitespace (View>Appearance>Render Whitespace). This shows all trailing white spaces in code that are not easily seen.

  • End-of-file-fixer: the hook checks that there should be an empty line at the very end of all files.

@Fiona-Waters
Copy link
Contributor Author

Excellent job on this PR! 👏

To fix the pre-commit checks:

  • Extra white spaces around the Readme file: if using VSCode what I like to do is activate Render Whitespace (View>Appearance>Render Whitespace). This shows all trailing white spaces in code that are not easily seen.
  • End-of-file-fixer: the hook checks that there should be an empty line at the very end of all files.

pre-commit now passing! Thanks @ChristianZaccaria

@Fiona-Waters
Copy link
Contributor Author

It'd be great to update the SDK e2e test and make sure it passes with Poetry:
https://github.com/project-codeflare/codeflare-operator/blob/6f7d6b1194dd88aa0b2ab08b5b0e1856793883dd/test/e2e/mnist_raycluster_sdk_test.go#L164

Thanks for the review @astefanutti Yes we should definitely do this. Do you think it would be okay if I create a new/follow on ticket for this, or would that need to be completed before merging?

@Fiona-Waters creating a follow up ticket to have it done along the next SDK dependency upgrade in the CodeFlare operator sounds the best course of action 👍🏼.

Ticket created - project-codeflare/codeflare-operator#250

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.

Ah I figured out why the unit tests are failing:

    import executing
E   ModuleNotFoundError: No module named 'executing'

In the pyproject.toml, executing should be a base dependency, not a test dependency. It's a package required in the SDK itself

@Fiona-Waters
Copy link
Contributor Author

Fiona-Waters commented Aug 30, 2023

Ah I figured out why the unit tests are failing:

    import executing
E   ModuleNotFoundError: No module named 'executing'

In the pyproject.toml, executing should be a base dependency, not a test dependency. It's a package required in the SDK itself

@Maxusmusti I was able to get the tests to pass by adding pip install executing here. This is more of a workaround than a fix I think. I am stumped otherwise as the module executing is listed in pyproject.toml and poetry.lock file. Please let me know what you think.

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 Tested installation and sdk whl file thats created

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 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.

Looks good, just one question about other places where the virtual env may need to be turned off. Also, the poetry.lock file needs to be updated once more to update the codeflare-torchx dependency version

.github/workflows/unit-tests.yml Show resolved Hide resolved
@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 Sep 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.

LGTM

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

openshift-ci bot commented Sep 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228, KPostOffice, Maxusmusti

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:
  • OWNERS [KPostOffice,Maxusmusti]

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

@openshift-merge-robot openshift-merge-robot merged commit 6c24d74 into project-codeflare:main Sep 14, 2023
2 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.

8 participants