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

Enable mcad_ray_test #144

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Enable mcad_ray_test #144

merged 1 commit into from
Oct 25, 2023

Conversation

ChristianZaccaria
Copy link
Contributor

Closes #143

Description

This test was being blocked by this issue project-codeflare/codeflare-sdk#190 which is now closed and merged.

@openshift-ci openshift-ci bot requested a review from jiripetrlik October 19, 2023 11:09
astefanutti
astefanutti reviewed Oct 19, 2023
View reviewed changes
tests/integration/mcad_ray_test.go
@@ -34,8 +34,6 @@ import (
func TestMCADRay(t *testing.T) {
test := cfosupport.With(t)

test.T().Skip("Requires https://github.com/project-codeflare/codeflare-sdk/issues/190")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's likely the Ray head node resources have to be customised with project-codeflare/codeflare-sdk#190.

@ChristianZaccaria ChristianZaccaria force-pushed the enable-mcad-ray-test branch 2 times, most recently from 4ba3ab1 to 3fc5c21 Compare October 23, 2023 13:38
@ChristianZaccaria
Copy link
Contributor Author

/retest

@ChristianZaccaria ChristianZaccaria force-pushed the enable-mcad-ray-test branch 2 times, most recently from 152da18 to bcaa0e8 Compare October 24, 2023 08:29
sutaakar
sutaakar reviewed Oct 24, 2023
View reviewed changes
tests/integration/resources/mnist_ray_mini.ipynb Outdated
@@ -40,7 +40,7 @@
"outputs": [],
"source": [
"# Create our cluster and submit appwrapper\n",
"cluster = Cluster(ClusterConfiguration(namespace=namespace, name='mnisttest', num_workers=1, min_cpus=1, max_cpus=1, min_memory=4, max_memory=4, num_gpus=0, instascale=False))"
"cluster = Cluster(ClusterConfiguration(namespace=namespace, name='mnisttest', head_cpus=2, head_memory=8, num_workers=1, min_cpus=1, max_cpus=1, min_memory=4, max_memory=4, num_gpus=0, instascale=False))"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reduce the head resources to some lower value?
IMHO head doesn't need too many resources, probably less than worker node.

@sutaakar
Copy link
Contributor

This one is hard to catch from logs:

File /opt/app-root/lib64/python3.9/site-packages/ray/_private/runtime_env/validation.py:131, in parse_and_validate_pip.<locals>._handle_local_pip_requirement_file(pip_file)
    129 pip_path = Path(pip_file)
    130 if not pip_path.is_file():
--> 131     raise ValueError(f"{pip_path} is not a valid file")
    132 return pip_path.read_text().strip().split("\n")

ValueError: /opt/app-root/notebooks-ray/requirements.txt is not a valid file

@sutaakar
Copy link
Contributor

Can you try to revert the notebooks-ray change? That should help.

@ChristianZaccaria
Copy link
Contributor Author

Can you try to revert the notebooks-ray change? That should help.

Good catch, I reverted now.

I noticed that all tests still make use of this import: "github.com/project-codeflare/codeflare-operator/test/support"

Should I update them to use "github.com/project-codeflare/codeflare-common/support" instead? Based on the recent Support Package Refactor

@sutaakar
Copy link
Contributor

@ChristianZaccaria well, you can do it as part of this PR or as a separate PR.
There are more changes to be done with that migration - leveraging implemented helper methods instead of current implementation in tests (i.e. creation of ConfigMaps, exposing service using Route/Ingress and such).

@astefanutti
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 25, 2023
@astefanutti
Copy link
Contributor

@ChristianZaccaria @sutaakar I'd vote to get this in and work on the refactoring in a separate PR.

sutaakar
sutaakar reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

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

/lgtm

@astefanutti
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti

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

@astefanutti
Copy link
Contributor

Thanks a lot @ChristianZaccaria!

@openshift-ci openshift-ci bot merged commit bd00817 into opendatahub-io:main Oct 25, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sutaakar sutaakar sutaakar left review comments

@astefanutti astefanutti astefanutti left review comments

@jiripetrlik jiripetrlik Awaiting requested review from jiripetrlik

Assignees

@astefanutti astefanutti

@sutaakar sutaakar

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable mcad_ray_test
3 participants
@ChristianZaccaria @sutaakar @astefanutti