-
Notifications
You must be signed in to change notification settings - Fork 694
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
🚀 Add HPO #2188
base: main
Are you sure you want to change the base?
🚀 Add HPO #2188
Conversation
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
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.
done with the initial round. I'll go over it again later
|
||
|
||
@dataclass | ||
class ParserConfig: |
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.
Not sure if I fully understand this object. Is it parsing objects? or storing them in one dataclass?
|
||
Args: | ||
project (str): The project name. | ||
entity (str | None): The entity or team name that is associated with the backend account. |
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 entity
what comet uses as an argument as well?
logger: "Logger" | ||
|
||
|
||
class CometParser: |
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.
If I follow the description in the docstring, would it be an idea to name this class as CometConfigParser
?
return CometParser(project=self.project, entity=self.entity) | ||
case HPOBackend.WANDB: | ||
msg = "wandb backend is not supported yet." | ||
raise ValueError(msg) |
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.
would it be an idea to replace this with NotImplementedError
) -> Generator[HPOJob, None, None]: | ||
"""Return iterator based on the arguments.""" | ||
del previous_stage_result # Not needed for this job | ||
assert config is not None, "Config is required for HPOJobGenerator." |
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.
Would it be possible to raise an error instead. Ruff does not like assert statements in the codebase
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
Signed-off-by: Ashwin Vaidya <ashwinnitinvaidya@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
==========================================
- Coverage 80.80% 80.48% -0.33%
==========================================
Files 248 255 +7
Lines 10859 11010 +151
==========================================
+ Hits 8775 8861 +86
- Misses 2084 2149 +65 ☔ View full report in Codecov by Sentry. |
📝 Description
TODO
✨ Changes
Select what type of change your PR is:
✅ Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.