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

SubmitRayJob enhancements #50

Merged
merged 18 commits into from
Aug 29, 2024
Merged

SubmitRayJob enhancements #50

merged 18 commits into from
Aug 29, 2024

Conversation

venkatajagannath
Copy link
Contributor

@venkatajagannath venkatajagannath commented Aug 24, 2024

This PR contains the following changes -

Enhancements to the SubmitRayJob operator

Based on customer feedback, we learnt that it would be a much easier UX to spin up/down the cluster in the background of a task. The user would simply decorate their python function with @ray.task and the decorator would orchestrate the rest.

To enable this feature, we had to make changes to the code for SetupRayCluster and DeleteRayCluster operators. Making these changes helps us avoid duplication.

Add more more example DAGs

Earlier we had only 2 example dags. We now have 4. And we execute a different DAG for integration test.

Making the Decorator more robust

We made some changes to the decorator source code to make it more robust

Unit tests updated

Added unit tests where necessary and deleted where unnecessary. Updated where required.

Documentation improvements

  • Significant changes to code samples section of the github page to make it easier to navigate
  • Added two additional code samples along with explanation
  • Added Getting Involved section to both Readme and Index.rst along with box formatting
  • Some other minor changes

Note: This PR does not contain any breaking changes.

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2024

Codecov Report

Attention: Patch coverage is 97.26776% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.80%. Comparing base (42a918d) to head (4d9c6b9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ray_provider/hooks/ray.py 96.36% 4 Missing ⚠️
ray_provider/decorators/ray.py 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   97.12%   97.80%   +0.68%     
==========================================
  Files           5        5              
  Lines         521      546      +25     
==========================================
+ Hits          506      534      +28     
+ Misses         15       12       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@venkatajagannath venkatajagannath added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 26, 2024
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

@venkatajagannath thanks for advancing on this and addressing customer's feedback!

Could you highlight in the PR description if/ what are the breaking changes of this PR?

Also, assuming there are breaking changes, would it make sense to release an alpha with this change (e.g. 0.2.0a1), so we can iterate with the customers on the desired interface/feature quickly? We can have as many alpha releases as we want without impacting other people

README.md Outdated Show resolved Hide resolved
@venkatajagannath
Copy link
Contributor Author

@venkatajagannath thanks for advancing on this and addressing customer's feedback!

Could you highlight in the PR description if/ what are the breaking changes of this PR?

Also, assuming there are breaking changes, would it make sense to release an alpha with this change (e.g. 0.2.0a1), so we can iterate with the customers on the desired interface/feature quickly? We can have as many alpha releases as we want without impacting other people

@tatiana There are no breaking changes in the release. It just simplifies cluster spin up and down. So in a way its actually an enhancement to an existing operator. Sure, we can do an alpha release.

Will update the PR description.

README.md Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
ray_provider/operators/ray.py Show resolved Hide resolved
ray_provider/operators/ray.py Show resolved Hide resolved
ray_provider/operators/ray.py Outdated Show resolved Hide resolved
ray_provider/operators/ray.py Show resolved Hide resolved
ray_provider/hooks/ray.py Show resolved Hide resolved
ray_provider/hooks/ray.py Show resolved Hide resolved
ray_provider/operators/ray.py Show resolved Hide resolved
ray_provider/operators/ray.py Show resolved Hide resolved
@pankajastro
Copy link
Collaborator

It just simplifies cluster spin up and down. So in a way its actually an enhancement to an existing operator. Sure, we can do an alpha release.

If this is urgent you can also cut alpha from this branch

pankajastro
pankajastro previously approved these changes Aug 29, 2024
Copy link
Collaborator

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

LGTM, There are minor breaking changes in theory we have documented them in the Changelog.

@venkatajagannath venkatajagannath merged commit cfbcec7 into main Aug 29, 2024
16 checks passed
@venkatajagannath venkatajagannath deleted the 3operatorsto1 branch August 29, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SubmitRayJob operator enhancements Add more Example DAGs
5 participants