Refactor AutoTuner as Python module #2487
Conversation
| return config | ||
|
|
||
| def read_tune(this): | ||
| from ray import tune |
There was a problem hiding this comment.
ray.tune() is pretty much used in most of the parsing anyways. is it a big overhead?
There was a problem hiding this comment.
It's already prepared for #2428, so importing ray.tune is not required when using functions from utils.
As Python does not import modules more than once, this line introduces minimal overhead at most.
| @@ -60,33 +53,20 @@ def test_algo_eval(self): | |||
| self.assertTrue(successful) | |||
|
|
|||
|
|
|||
| class ASAP7AlgoEvalSmokeTest(BaseAlgoEvalSmokeTest): | |||
| platform = "asap7" | |||
There was a problem hiding this comment.
noticed the removal of make_base() and test_*(). does the tests still run?
i am aware that the autotuner tests are disabled now, but just wanted to double check if tests can still trigger
There was a problem hiding this comment.
Yes, tests still run, they were triggered in some previous CIs, eg. for GCD. I've just reduced code duplication by propagate these methods with inheritance
tools/AutoTuner/pyproject.toml
Outdated
| "License :: OSI Approved :: BSD 3-Clause", | ||
| ] | ||
| readme = "README.md" | ||
| dependencies = [ |
There was a problem hiding this comment.
potentially consider the separation of requirements and requirements-test
[tool.setuptools.dynamic]
dependencies = { file = ["requirements.txt"] }
optional-dependencies = { dev = { file = ["requirements-test.txt"] } }
There was a problem hiding this comment.
Great idea, but are there any test-specific dependencies? From what I've saw only additional dependency in tests is unittest, which is Python built-in module.
Also, is there a specific reason to store dependencies in separate requirements files instead of pyproject.toml (which is default way I believe)?
There was a problem hiding this comment.
No particular reason. Just that we want to make it compatible with package locking in the future. (i.e. pip-compile/ poetry etc).
It might not be called requirements-test.txt but rather requirements-dev.txt, where we want to split out only the core minimal libraries for production vs for dev.
There was a problem hiding this comment.
Sure, I moved dependencies to separate files - for now requirements-dev.txt have only black formatter.
eszpotanski
force-pushed
the
eszpot/autotuner-refactoring
branch
from
664e8f5
to
256ce2d
Compare
Signed-off-by: Eryk Szpotanski <[email protected]>
Signed-off-by: Eryk Szpotanski <[email protected]>
Signed-off-by: Eryk Szpotanski <[email protected]>
Signed-off-by: Eryk Szpotanski <[email protected]>
eszpotanski
force-pushed
the
eszpot/autotuner-refactoring
branch
from
256ce2d
to
fe29652
Compare
There was a problem hiding this comment.
Approval is pending smoke test re-enablement @vvbandeira fyi.
@eszpotanski No action needed for now, for this PR
|
FWIW, I've run the PR through some local quick testing and it appears to work for the non-cluster tune and sweep mode tests that I ran (not exhaustive, of course). I've also started to apply my own re-factoring on top of this PR, so that I can send those up for review after this PR gets merged. |
This PR changes the structure of AutoTuner, so it can be used as a Python module. It also move part of the code (not closely related to tuning) to separate
utilsfile.