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

[GSoC] Add unit tests for tune API #2423

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

helenxie-bit
Copy link
Contributor

@helenxie-bit helenxie-bit commented Sep 5, 2024

What this PR does / why we need it:
This PR adds unit tests and fix some minor bugs of tune API.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Checklist:

  • Docs included if any changes are user facing

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit
Copy link
Contributor Author

helenxie-bit commented Sep 5, 2024

/area gsoc

@helenxie-bit
Copy link
Contributor Author

Ref: #2339

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit helenxie-bit changed the title [GSoC] Add unit tests for tune API [WIP] Add unit tests for tune API Sep 5, 2024
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for doing this @helenxie-bit!

test/unit/v1beta1/tune-api/test_tune_api.py Outdated Show resolved Hide resolved
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@google-oss-prow google-oss-prow bot added size/L and removed size/XL labels Sep 9, 2024
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit helenxie-bit changed the title [WIP] Add unit tests for tune API [GSoC] Add unit tests for tune API Sep 11, 2024
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit
Copy link
Contributor Author

@andreyvelich The unit test for tune API is ready. I have also fixed some errors in this PR. Please have a look when you have time. Thanks!

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

…yword argument

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from johnugeorge. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit
Copy link
Contributor Author

@andreyvelich Thank you so much for reviewing this! I’ve replied to all your feedback, and please take a look when you have time.

Copy link
Member

@Electronic-Waste Electronic-Waste left a comment

Choose a reason for hiding this comment

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

Thanks for your great contribution @helenxie-bit ! I left some suggestions about the name of UTs for your reference

I guess it will be more friendly for other developers to understand these UTs if we could specify the scope of each UTs:)

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit
Copy link
Contributor Author

Thanks for your great contribution @helenxie-bit ! I left some suggestions about the name of UTs for your reference

I guess it will be more friendly for other developers to understand these UTs if we could specify the scope of each UTs:)

@Electronic-Waste Thank you so much for your suggestions! I have incorporated your advice into the UTs.

…ing[huggingface' into dependencies

Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
Signed-off-by: helenxie-bit <helenxiehz@gmail.com>
@helenxie-bit
Copy link
Contributor Author

@andreyvelich I’ve added the kubeflow-training dependency to the unit tests, allowing values and parameters to be fetched from there instead of redefining them within the Katib unit tests. You can find the change here:

kubeflow-training[huggingface]==1.8.1

Additionally, I have added verification for the experiment object in two valid flow cases in the following section:
call_args = mock_create_experiment.call_args
experiment = call_args[0][0]
if test_name == "valid flow with custom objective tuning":
# Verify input_params
args_content = "".join(
experiment.spec.trial_template.trial_spec.spec.template.spec.containers[
0
].args
)
assert "'a': '${trialParameters.a}'" in args_content
# Verify trial_params
assert experiment.spec.trial_template.trial_parameters == [
V1beta1TrialParameterSpec(name="a", reference="a"),
]
# Verify experiment_params
assert experiment.spec.parameters == [
V1beta1ParameterSpec(
name="a",
parameter_type="int",
feasible_space=V1beta1FeasibleSpace(min="10", max="100"),
),
]
elif test_name == "valid flow with external model tuning":
# Verify input_params
args_content = "".join(
experiment.spec.trial_template.trial_spec.spec.pytorch_replica_specs[
"Master"
]
.template.spec.containers[0]
.args
)
assert (
'"learning_rate": "${trialParameters.learning_rate}"'
in args_content
)
# Verify trial_params
assert experiment.spec.trial_template.trial_parameters == [
V1beta1TrialParameterSpec(
name="learning_rate", reference="learning_rate"
),
]
# Verify experiment_params
assert experiment.spec.parameters == [
V1beta1ParameterSpec(
name="learning_rate",
parameter_type="double",
feasible_space=V1beta1FeasibleSpace(
min="1e-05", max="5e-05"
),
),
]

Please review when you have time. Thanks!

@helenxie-bit
Copy link
Contributor Author

@andreyvelich Please review this PR when you have time 👀. I'll adjust the e2e tests for the tune API after this PR is merged, as it fixes some small bugs in the tune API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants