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

[SDK] Support More MCKind in tune #2402

Open
2 of 6 tasks
Electronic-Waste opened this issue Aug 3, 2024 · 11 comments · May be fixed by #2406
Open
2 of 6 tasks

[SDK] Support More MCKind in tune #2402

Electronic-Waste opened this issue Aug 3, 2024 · 11 comments · May be fixed by #2406
Assignees
@prakhar479
Labels
area/sdk good first issue Good for newcomers help wanted Extra attention is needed kind/feature

Comments

@Electronic-Waste
Copy link
Member

Electronic-Waste commented Aug 3, 2024 •
edited
Loading

What you would like to be added?

Support specifying more types of metrics collector in the tune function.

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Line 189 in 51b246f

metrics_collector_config: Dict[str, Any] = {"kind": "StdOut"},

Why is this needed?

Currently, we only support kind param in metrics_collector_config, which means only StdOut and Push collector can be specified in tune function.

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Lines 387 to 391 in 51b246f

# Add metrics collector to the Katib Experiment.
# Up to now, We only support parameter `kind`, of which default value is `StdOut`, to specify the kind of metrics collector.
experiment.spec.metrics_collector = models.V1beta1MetricsCollectorSpec(
collector=models.V1beta1CollectorSpec(kind=metrics_collector_config["kind"])
)

However, there are many other collector types, such as File, Prometheus, Custom, TensorFlowEvent collector. They are important too. The tune function would be more flexible and powerful if users can specify all these kind of collectors.

Currently we support:

Love this feature?

Give it a 👍 We prioritize the features with most 👍
@Electronic-Waste
Copy link
Member Author

Electronic-Waste commented Aug 3, 2024 •
edited
Loading

/remove-label lifecycle/needs-triage

/area sdk

@google-oss-prow Google OSS Prow
Copy link

@Electronic-Waste: The label(s) `/remove-label lifecycle/needs-triage

cannot be applied. These labels are supported:tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash. Is this label configured under labels -> additional_labelsorlabels -> restricted_labelsinplugin.yaml`?

In response to this:

/remove-label lifecycle/needs-triage

/area sdk

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@Electronic-Waste
Copy link
Member Author

/good-first-issue

@google-oss-prow Google OSS Prow
Copy link

@Electronic-Waste:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@google-oss-prow google-oss-prow bot added good first issue Good for newcomers help wanted Extra attention is needed labels Aug 3, 2024
@prakhar479
Copy link

prakhar479 commented Aug 6, 2024 •
edited
Loading

hey @Electronic-Waste I wanted to work on this issue and have some doubts regarding the same. Are the only modifications required in tune function to allow other function signature or does V1beta1CollectorSpec method from models class would also need some changes to handle these different collector types. Also, what is the specific representation (string key) for these other collector types (any example would be helpful). Thanks!

@Electronic-Waste
Copy link
Member Author

Hi @prakhar479 , thank you for your passion for this issue!

I think V1beta1CollectorSpec is already up-to-date to handle different types of MC. So probably you only need to add some fields to metrics_collector_config and modify these lines of code in tune function.

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Lines 387 to 391 in b6f7cfd

# Add metrics collector to the Katib Experiment.
# Up to now, We only support parameter `kind`, of which default value is `StdOut`, to specify the kind of metrics collector.
experiment.spec.metrics_collector_spec = models.V1beta1MetricsCollectorSpec(
collector=models.V1beta1CollectorSpec(kind=metrics_collector_config["kind"])
)

Also, you may find these examples helpful to you :)

https://github.com/kubeflow/katib/blob/master/examples/v1beta1/metrics-collector/custom-metrics-collector.yaml
https://github.com/kubeflow/katib/blob/master/examples/v1beta1/metrics-collector/file-metrics-collector-with-json-format.yaml
https://github.com/kubeflow/katib/blob/master/examples/v1beta1/metrics-collector/file-metrics-collector.yaml

cc👀 @kubeflow/wg-automl-leads

@Electronic-Waste
Copy link
Member Author

/assign @prakhar479

@prakhar479
Copy link

thanks for the helpful insight @Electronic-Waste. I wanted to confirm one doubt I encountered while going through the code.
V1beta1CollectorSpec method has a custom_collector: Any | None = None field which is not being used currently in the initialization. can we set up extra fields in the metrics_collector_config for other types of collectors and then pass them in custom_collector field in line 390

@prakhar479
Copy link

Also, can someone lead me to a guide to set up and run tests for the repo. I searched around and was able to get few things going but couldn't run e2e tests due to incomplete set up on my end. Thanks!

@Electronic-Waste
Copy link
Member Author

can we set up extra fields in the metrics_collector_config for other types of collectors and then pass them in custom_collector field in line 390

@prakhar479 Yes, of course. You can add any fields you want in metrics_collector_config.

Also, can someone lead me to a guide to set up and run tests for the repo. I searched around and was able to get few things going but couldn't run e2e tests due to incomplete set up on my end. Thanks!

In fact, I run the e2e tests by submitting PRs to this repo. E2e tests will be triggered automatically in CI/CD. You can also consult @andreyvelich @tenzen-y @johnugeorge for more suggestions.

@prakhar479 prakhar479 linked a pull request Aug 9, 2024 that will close this issue
Open
@Electronic-Waste
Copy link
Member Author

/remove-label lifecycle/needs-triage

@Electronic-Waste Electronic-Waste mentioned this issue Sep 19, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prakhar479 prakhar479

Labels
area/sdk good first issue Good for newcomers help wanted Extra attention is needed kind/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

* refactor(sdk): added option for custom metric collector for tune in… prakhar479/katib
2 participants
@Electronic-Waste @prakhar479