Small fixes #964
Merged
Small fixes #964
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lebrice
reviewed
Jul 7, 2022
| producer = Producer(experiment) | ||
|
|
||
| experiment_client = ExperimentClient(experiment, producer) | ||
| experiment_client = ExperimentClient(experiment) |
There was a problem hiding this comment.
Yes exactly! I found the same bug thanks to type hints on my end yesterday:
|
The pre-commit hook complains about things that did not change in this PR. Should I just ignore it? |
4 tasks
|
It's weird that these errors do not show up in the CI for the dev branch. If you are able to fix these it would be great. Otherwise we'll do it in another PR. |
|
I did a close/reopen to re-run the tests |
bouthilx
added
the
enhancement
lebrice
added a commit
to lebrice/orion
that referenced
this pull request
Jul 29, 2022
Signed-off-by: Fabrice Normandin <[email protected]>
Merged
lebrice
added a commit
that referenced
this pull request
Sep 30, 2022
* Import files from old PR, start fresh Signed-off-by: Fabrice Normandin <[email protected]> * Cleaning up algo_wrapper.py Signed-off-by: Fabrice Normandin <[email protected]> * Minor typing improvements in utils/__init__.py Signed-off-by: Fabrice Normandin <[email protected]> * add transform-related methods to AlgoWrapper Signed-off-by: Fabrice Normandin <[email protected]> * Clean up multi_task_algo.py Signed-off-by: Fabrice Normandin <[email protected]> * [big] move wrappers to folder, simplify wrappers Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for the wrappers Signed-off-by: Fabrice Normandin <[email protected]> * Move algo wrappers to folder, split up tests a bit Signed-off-by: Fabrice Normandin <[email protected]> * Fix cyclical import issue with knowledbe base Signed-off-by: Fabrice Normandin <[email protected]> * Remove accidentally added .pre-commit-config.yaml Signed-off-by: Fabrice Normandin <[email protected]> * "algorithms.algorithm" -> "algorithms.unwrapped" Signed-off-by: Fabrice Normandin <[email protected]> * Have wrappers return the algo's config, not theirs Signed-off-by: Fabrice Normandin <[email protected]> * Remove duplicated code from SpaceTransform Signed-off-by: Fabrice Normandin <[email protected]> * Rename test files for algo wrappers Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug in test_gridsearch.py Signed-off-by: Fabrice Normandin <[email protected]> * Remove more duplicated code from SpaceTransform Signed-off-by: Fabrice Normandin <[email protected]> * Husk of unit tests for multi-task wrapper Signed-off-by: Fabrice Normandin <[email protected]> * Rename multi_task_wrapper.py -> multi_task.py Signed-off-by: Fabrice Normandin <[email protected]> * Start to write body of the test Signed-off-by: Fabrice Normandin <[email protected]> * Remove other duplicate methods of SpaceTransform Signed-off-by: Fabrice Normandin <[email protected]> * Add dataclasses for various objects Signed-off-by: Fabrice Normandin <[email protected]> * Adapting previous changes from config_dataclasses Signed-off-by: Fabrice Normandin <[email protected]> * Adapting previous changes from config_dataclasses Signed-off-by: Fabrice Normandin <[email protected]> * Fix / reformat docstrings Signed-off-by: Fabrice Normandin <[email protected]> * Fix typo in log.debug call Co-authored-by: Xavier Bouthillier <[email protected]> * Fix outdated docstrings Signed-off-by: Fabrice Normandin <[email protected]> * Revert changes to ugly part of testing/__init__.py Signed-off-by: Fabrice Normandin <[email protected]> * Fix isort issues Signed-off-by: Fabrice Normandin <[email protected]> * Fix isort / flake8 issues Signed-off-by: Fabrice Normandin <[email protected]> * Add an intermediate TransformWrapper ABC Signed-off-by: Fabrice Normandin <[email protected]> * Add optional `max_trials` property to algorithm Signed-off-by: Fabrice Normandin <[email protected]> * Remove outdated/misleading comments Signed-off-by: Fabrice Normandin <[email protected]> * Fix various pylint/flake8 errors Signed-off-by: Fabrice Normandin <[email protected]> * Touchups on the types of primary_algo.py Signed-off-by: Fabrice Normandin <[email protected]> * Slight refactoring in serializable.py Signed-off-by: Fabrice Normandin <[email protected]> * Rename AlgoType TypeVar to AlgoT Signed-off-by: Fabrice Normandin <[email protected]> * Start to add tests for ExperimentInfo dataclass Signed-off-by: Fabrice Normandin <[email protected]> * Allow passing Algo type to create_experiment Signed-off-by: Fabrice Normandin <[email protected]> * Minor tweak in AlgoWrapper.__repr__ Signed-off-by: Fabrice Normandin <[email protected]> * Adding (failing) tests for warm-starting. Signed-off-by: Fabrice Normandin <[email protected]> * Add Knowledge Base arg to Experiment and Producer Signed-off-by: Fabrice Normandin <[email protected]> * Add KnowledgeBase as argument to workon functions Signed-off-by: Fabrice Normandin <[email protected]> * Move warm-start method to right wrapper class Signed-off-by: Fabrice Normandin <[email protected]> * Add a simple test for warm starting Signed-off-by: Fabrice Normandin <[email protected]> * Improve __repr__ of Registry Signed-off-by: Fabrice Normandin <[email protected]> * Type the `space` property of ExperimentClient Signed-off-by: Fabrice Normandin <[email protected]> * Remove line that caused bug in experiment_builder Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for KB + Warm-Starting Signed-off-by: Fabrice Normandin <[email protected]> * (big, ugly commit) Add tests, clarify, refactor Signed-off-by: Fabrice Normandin <[email protected]> * Add repr for RegistryMapping Signed-off-by: Fabrice Normandin <[email protected]> * Type the _results and _params attributes of Trial Signed-off-by: Fabrice Normandin <[email protected]> * Move and Rename algo for unit tests Signed-off-by: Fabrice Normandin <[email protected]> * Add test for Multi-Task wrapper collisions Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug in AlgoWrapper (see desc.) Subclasses of AlgoWrapper (in particular the InsistSuggest wrapper) did not register the trials into their registry when suggesting or observing. This caused an issue where the registry mapping wasn't working properly in the MultiTaskWrapper. - Made the `suggest` and `observe` of AlgoWrapper use `self.register` so that the registry mapping and collision detection now works properly. Signed-off-by: Fabrice Normandin <[email protected]> * Add test for suggest to always give task_id=0 Signed-off-by: Fabrice Normandin <[email protected]> * Fix small bug in gridsearch.py Signed-off-by: Fabrice Normandin <[email protected]> * Simplification: kb is only attr of Experiment Signed-off-by: Fabrice Normandin <[email protected]> * Moved "functional" tests to different file Signed-off-by: Fabrice Normandin <[email protected]> * Fix pylint errors Signed-off-by: Fabrice Normandin <[email protected]> * Add unwrap convenience method on AlgoWrapper Signed-off-by: Fabrice Normandin <[email protected]> * Misc changes (test cleanup, copy status) Signed-off-by: Fabrice Normandin <[email protected]> * Remove unused warm_start_mode context manager Signed-off-by: Fabrice Normandin <[email protected]> * Use the new max_trials property on algo Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for setting max_trials and n_observed Signed-off-by: Fabrice Normandin <[email protected]> * Fix renaming of SpaceTransform algo wrapper Signed-off-by: Fabrice Normandin <[email protected]> * Don't register trials from other tasks in algo Signed-off-by: Fabrice Normandin <[email protected]> * Fix randomness of flaky-ish test Signed-off-by: Fabrice Normandin <[email protected]> * Remove dict[k, v] type annotation for python < 3.9 Signed-off-by: Fabrice Normandin <[email protected]> * Move / Simplify the Config classes -> TypedDicts Signed-off-by: Fabrice Normandin <[email protected]> * Type out the Random algo, fix a test Signed-off-by: Fabrice Normandin <[email protected]> * Fixing some broken tests Signed-off-by: Fabrice Normandin <[email protected]> * Fix experiment_builder tests Signed-off-by: Fabrice Normandin <[email protected]> * Fix broken test in test_experiment.py Signed-off-by: Fabrice Normandin <[email protected]> * Add test for ExperimentConfig fields Signed-off-by: Fabrice Normandin <[email protected]> * Minor touchups in experiment_config.py Signed-off-by: Fabrice Normandin <[email protected]> * Removed unused typeddicts in experiment_config.py Signed-off-by: Fabrice Normandin <[email protected]> * Remove outdated todo Signed-off-by: Fabrice Normandin <[email protected]> * [breaking] Knowledge Base implementation Signed-off-by: Fabrice Normandin <[email protected]> * Remove unused knowledge_base argument to workon Signed-off-by: Fabrice Normandin <[email protected]> * Fix instantiation of KB in exp builder Signed-off-by: Fabrice Normandin <[email protected]> * [nit] Fix typing of ExperimentStats fields Signed-off-by: Fabrice Normandin <[email protected]> * [optional] Type out storage/base.py and misc types Signed-off-by: Fabrice Normandin <[email protected]> * Adapting the MultiTask wrapper tests, add stubs Signed-off-by: Fabrice Normandin <[email protected]> * Move test_experiment_config to reflect src Signed-off-by: Fabrice Normandin <[email protected]> * Add test for KB Signed-off-by: Fabrice Normandin <[email protected]> * Add more tests for the KnowledgeBase Signed-off-by: Fabrice Normandin <[email protected]> * Fix incorrect type for experiment id in docstrings Signed-off-by: Fabrice Normandin <[email protected]> * Fix error in fixture, adjust docstrings Signed-off-by: Fabrice Normandin <[email protected]> * Pass experiment config instead of experiment obj Signed-off-by: Fabrice Normandin <[email protected]> * Fix docstrings, use Unpack[] to type **kwargs Signed-off-by: Fabrice Normandin <[email protected]> * Move and update functional tests Signed-off-by: Fabrice Normandin <[email protected]> * Remove unused code block in test Signed-off-by: Fabrice Normandin <[email protected]> * Add note about the Unpack[] annotation Signed-off-by: Fabrice Normandin <[email protected]> * Add PartialExperimentConfig typeddict Signed-off-by: Fabrice Normandin <[email protected]> * Trying to make functional tests pass... Signed-off-by: Fabrice Normandin <[email protected]> * Fix pylint error in experiment.py Signed-off-by: Fabrice Normandin <[email protected]> * Fix bugs in test_knowledge_base Signed-off-by: Fabrice Normandin <[email protected]> * Pass kb to instantiate_algo, minor typing stuffs Signed-off-by: Fabrice Normandin <[email protected]> * Clarify potential issue in register, touchups Signed-off-by: Fabrice Normandin <[email protected]> * Minor typing touchups Signed-off-by: Fabrice Normandin <[email protected]> * Simplify functional tests for warm_starting Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for how to pass the algorithm Signed-off-by: Fabrice Normandin <[email protected]> * Minor typing improvements to experiment_builder.py Signed-off-by: Fabrice Normandin <[email protected]> * Fix assignment of max_trials in exp client Signed-off-by: Fabrice Normandin <[email protected]> * Use KnowledgeBase in functional test, remove todos Signed-off-by: Fabrice Normandin <[email protected]> * Fix pylint error Signed-off-by: Fabrice Normandin <[email protected]> * Fix import error for TypedDict Signed-off-by: Fabrice Normandin <[email protected]> * Re-introduce fix from #964 (?) Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug in algo creation logic Signed-off-by: Fabrice Normandin <[email protected]> * Add missing types in BaseAlgorithm Signed-off-by: Fabrice Normandin <[email protected]> * Add some of the missing types in exp builder Signed-off-by: Fabrice Normandin <[email protected]> * Remove leftover todo in BaseAlgorithm Signed-off-by: Fabrice Normandin <[email protected]> * Fix type annotation on create_experiment Signed-off-by: Fabrice Normandin <[email protected]> * Remove extra type annotation on create_experiment Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug in test_tpe (added wrapper) Signed-off-by: Fabrice Normandin <[email protected]> * Fix value in DumbAlgo Signed-off-by: Fabrice Normandin <[email protected]> * Fix test condition Signed-off-by: Fabrice Normandin <[email protected]> * Remove fixme comment Signed-off-by: Fabrice Normandin <[email protected]> * Fix tests Signed-off-by: Fabrice Normandin <[email protected]> * Fix most PBT tests Signed-off-by: Fabrice Normandin <[email protected]> * Misc changes Signed-off-by: Fabrice Normandin <[email protected]> * Misc typing changes Signed-off-by: Fabrice Normandin <[email protected]> * Debugging the PBT Errors Signed-off-by: Fabrice Normandin <[email protected]> * Fix broken AlgoWrapper tests Signed-off-by: Fabrice Normandin <[email protected]> * Add missing register method on AlgoWrapper Signed-off-by: Fabrice Normandin <[email protected]> * Remove unused `original_space` property Signed-off-by: Fabrice Normandin <[email protected]> * Fix docstring and warning in BaseAlgorithm.get_id Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug introduced previously Signed-off-by: Fabrice Normandin <[email protected]> * Minor improvements to TransformWrapper.suggest Signed-off-by: Fabrice Normandin <[email protected]> * Use self.reverse_transform in get_original_parent Signed-off-by: Fabrice Normandin <[email protected]> * Ugly temporary fix to bug that affected PBT (desc) There is a weird bug that was happening in PBT: - The trials that PBT suggest have a parent id. - When the SpaceTransform wrapper calls observe, the completed trials don't have the same parent as they did when they were suggested. Signed-off-by: Fabrice Normandin <[email protected]> * Remove hacky fix, fix bug source (hopefully) Signed-off-by: Fabrice Normandin <[email protected]> * Make _get_original_parent "static" again Signed-off-by: Fabrice Normandin <[email protected]> * Add todo for later (copying attributes explicitly) Signed-off-by: Fabrice Normandin <[email protected]> * Fix minor bug in DumbAlgo-related test Signed-off-by: Fabrice Normandin <[email protected]> * Greatly reduce number of warnings Signed-off-by: Fabrice Normandin <[email protected]> * Remove Registry from InsistSuggestWrapper Signed-off-by: Fabrice Normandin <[email protected]> * Add tests to increase coverage of Registry class Signed-off-by: Fabrice Normandin <[email protected]> * Add more coverage for _instantiate_knowledge_base Signed-off-by: Fabrice Normandin <[email protected]> * Add a bit more coverage for AlgoWrapper Signed-off-by: Fabrice Normandin <[email protected]> * Fix bug in _instantiate_knowledge_base Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for _instantiate_algo Signed-off-by: Fabrice Normandin <[email protected]> * Add generic test class for AlgoWrappers Signed-off-by: Fabrice Normandin <[email protected]> * Remove redundant fixture in AlgoWrapper tests Signed-off-by: Fabrice Normandin <[email protected]> * Clean / minor changes to test_knowledge_base.py Signed-off-by: Fabrice Normandin <[email protected]> * Use asserts to avoid writing useless tests Signed-off-by: Fabrice Normandin <[email protected]> * Add test case for no compatible trials found Signed-off-by: Fabrice Normandin <[email protected]> * Clean up testing utility function a bit Signed-off-by: Fabrice Normandin <[email protected]> * Add test for not warm-starting twice Signed-off-by: Fabrice Normandin <[email protected]> * Add test for "space already has task_id" error Signed-off-by: Fabrice Normandin <[email protected]> * Add tests for is_warmstarteable function Signed-off-by: Fabrice Normandin <[email protected]> * Standardize the imports of ExperimentConfig Signed-off-by: Fabrice Normandin <[email protected]> * Add more tests for _instantiate_kb Signed-off-by: Fabrice Normandin <[email protected]> * Remove redundante else clause in _instantiate_algo Signed-off-by: Fabrice Normandin <[email protected]> Signed-off-by: Fabrice Normandin <[email protected]> Signed-off-by: Fabrice Normandin <[email protected]> Co-authored-by: Fabrice Normandin <[email protected]> Co-authored-by: Xavier Bouthillier <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a collection of small fixes for bugs I found while investigating the code.
If one of the fixes is questionable, I can remove the commit in question from the PR on request.
Checklist
Documentation
Quality
$ tox -e lint)