-
Notifications
You must be signed in to change notification settings - Fork 254
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
Adds comet-ml plugin #2550
Adds comet-ml plugin #2550
Conversation
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2550 +/- ##
===========================================
+ Coverage 52.89% 90.41% +37.52%
===========================================
Files 297 60 -237
Lines 23957 3058 -20899
Branches 3655 0 -3655
===========================================
- Hits 12671 2765 -9906
+ Misses 11178 293 -10885
+ Partials 108 0 -108 ☔ View full report in Codecov by Sentry. |
# For local execution, always use the experiment_key. If `self.experiment_key` is `None`, comet_ml | ||
# will generate it's own key | ||
if self.experiment_key is not None: | ||
init_kwargs["experiment_key"] = self.experiment_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is api key not required for local executions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required, I use export COMET_API_KEY="xxxx"
to test it in local execution.
raise ValueError("project_name must be set") | ||
if workspace is None: | ||
raise ValueError("workspace must be set") | ||
if secret is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't secret
be made optional as it isn't necessary when executing locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The secret field is ignored during location execution, but required during remote. One still needs export COMET_API_KEY=
for local execution.
If this was optional, we'll catch the "no secret" during execution and not at registration time. I do not want to wait until then, because these experiments can use GPUs and waiting & spinning up a GPU just to see this error is not efficient and a worst DevX.
By making secret
required, I am optimizing for "failing quickly" for remote execution.
Signed-off-by: Thomas J. Fan <[email protected]>
…ml_init Signed-off-by: Thomas J. Fan <[email protected]>
plugins/flytekit-comet-ml/README.md
Outdated
dynamic-log-links: | ||
- comet-ml-execution-id: | ||
displayName: Comet | ||
templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .executionName }}{{ .nodeId }}{{ .taskRetryAttempt }}{{ .taskConfig.link_suffix }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use double quote here, right?
plugins/flytekit-comet-ml/README.md
Outdated
templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .executionName }}{{ .nodeId }}{{ .taskRetryAttempt }}{{ .taskConfig.link_suffix }} | ||
- comet-ml-custom-id: | ||
displayName: Comet | ||
templateUris: {{ .taskConfig.host }}/{{ .taskConfig.workspace }}/{{ .taskConfig.project_name }}/{{ .taskConfig.experiment_key }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing
project_name: Optional[str] = None, | ||
workspace: Optional[str] = None, | ||
experiment_key: Optional[str] = None, | ||
secret: Optional[Union[Secret, Callable]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can project_name, workspace, and secret be None Optional
here in the type hints?
It will be better for the type hints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are already Optional
here. Are you suggesting to not make them Optional
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it to work with 417d03b It forces project_name
and friends to be required.
@thomasjpfan |
Signed-off-by: Thomas J. Fan <[email protected]>
0eb34fa
to
cc2e9ad
Compare
* Adds comet-ml plugin Signed-off-by: Thomas J. Fan <[email protected]> * For local execution, do not set experiment_key if it is none Signed-off-by: Thomas J. Fan <[email protected]> * Use correct comet-ml links Signed-off-by: Thomas J. Fan <[email protected]> * Allow host to be adjustable Signed-off-by: Thomas J. Fan <[email protected]> * Adds comet-ml plugin Signed-off-by: Thomas J. Fan <[email protected]> * Use new comet-ml login name Signed-off-by: Thomas J. Fan <[email protected]> * Require the project_name workspace and secrets Signed-off-by: Thomas J. Fan <[email protected]> --------- Signed-off-by: Thomas J. Fan <[email protected]> Signed-off-by: mao3267 <[email protected]>
Why are the changes needed?
This PR adds a comet-ml plugin to flytekit.
What changes were proposed in this pull request?
This PR adds a
comet_ml_init
task decorator that adds a link to the Comet's platform.How was this patch tested?
Unit tests were added to this PR.
Screenshots
Related PRs
Related to #2405
Docs link
I also added an example in flytesnacks: flyteorg/flytesnacks#1702