fix: Fix import bug, and also new workflow to install Syne Tune with … #663
Conversation
…gpsearchers dependencies only
|
I'll check which of the examples should also run with minimal dependencies, and change the workflow there as well. Also makes the CI run faster. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
=======================================
Coverage 66.01% 66.01%
=======================================
Files 381 381
Lines 26915 26917 +2
=======================================
+ Hits 17768 17770 +2
Misses 9147 9147
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The .github/workflows/run-syne-tune.yml is a re-usable workflow, and you can pass in custom inputs at the top of the file.
Rather than creating this new re-usable workflow, I'd recommend creating a new option in the original file which specifies the mode that you want to set.
Something perhaps along the lines of:
on:
workflow_call:
inputs:
gpsearchers-only:
required: false
type: boolean
And then update the logic to branch based on that flag.
There was a problem hiding this comment.
OK, that is indeed better, I'll do that
| extras-require: | ||
| required: false | ||
| type: string |
| # object should fail in this case. | ||
| class _MultiObjectiveMixedVariableProblem(Problem): | ||
| def __init__(self, n_obj: int, config_space: Dict[str, Any], **kwargs): | ||
| vars = {} | ||
|
|
||
| for hp_name, hp in config_space.items(): | ||
| if isinstance(hp, Domain): | ||
| if isinstance(hp, Categorical): | ||
| vars[hp_name] = PyMOOChoice(options=hp.categories) | ||
| elif isinstance(hp, Integer): | ||
| vars[hp_name] = PyMOOInteger(bounds=(hp.lower, hp.upper - 1)) | ||
| elif isinstance(hp, FiniteRange): | ||
| vars[hp_name] = PyMOOInteger(bounds=(0, hp.size - 1)) | ||
| elif isinstance(hp, Float): | ||
| vars[hp_name] = PyMOOReal(bounds=(hp.lower, hp.upper)) | ||
| else: | ||
| raise Exception( | ||
| f"Type {type(hp)} of hyperparameter {hp_name} is not supported!" |
There was a problem hiding this comment.
The current change of nesting this in a class makes sense to fix the import issue.
But all these isinstance() checks added in #660 feel somehow brittle to me, swapping in all these PyMOO datatypes here. I wonder if there's a cleaner way to use MOO, or if using MOO necessarily introduces this complexity.
Why does PyMOO even create those special pymoo datatypes?
There was a problem hiding this comment.
(not blocking this mitigation PR, more of a general comment)
Approved
There was a problem hiding this comment.
Well, fair enough, we also create our own datatypes!
There was a problem hiding this comment.
I think this is fine. As long as we remember to keep this stuff in functions, so it is not imported when not really needed. And the new integ tests will make sure this does not happen
|
Thanks for improving the testing as well to use narrower imports, this is really good! |
…gpsearchers dependencies only
run-syne-tune-gpsearchers-only.yml, and make sure thatexamples/launch_standalone_bayesian_optimization.pyruns with this one (would have caught the bug)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.