[Tabular] Resource Allocation Fix #2536
Conversation
| user_specified_lower_level_num_cpus = min(user_specified_model_level_num_cpus * k_fold, system_num_cpus) | ||
| if user_specified_lower_level_num_gpus is not None: | ||
| if user_specified_model_level_num_gpus is not None: | ||
| user_specified_lower_level_num_gpus = min(user_specified_lower_level_num_gpus * k_fold, user_specified_model_level_num_gpus) |
There was a problem hiding this comment.
Is this correct?
Why are we changing the user specified num_gpus for ag_args_fit_ensemble?
Why are we multiplying ag_args_fit_ensemble by k_fold??
There was a problem hiding this comment.
Please don't mark as resolved without responding with an answer. This is a major bug if it is indeed a bug.
Another question would be: Why didn't the unit tests catch this bug?
| @@ -190,7 +210,7 @@ def test_nonbagged_model_with_total_resources_and_model_resources(mock_system_re | |||
| hyperparameters={ | |||
| 'ag_args_fit': { | |||
| 'num_cpus': 1, | |||
| 'num_gpus': 1 | |||
| 'num_gpus': 0 | |||
There was a problem hiding this comment.
Why is this hard-coded? Wouldn't it be way easier to just have this be in the arguments of the test?
tabular/tests/unittests/resource_allocation/test_total_resource_allocation.py
Outdated
Show resolved
Hide resolved
| model_base = DummyModel() | ||
| bagged_model = DummyBaggedModel( | ||
| model_base, | ||
| hyperparameters={} |
There was a problem hiding this comment.
Why is this hard-coded? Wouldn't it be way easier to just have this be in the arguments of the test?
This way we can specify ag_args_fit_ensemble for example.
Example for ag_args_fit_ensemble:
hyperparameters={
'ag_args_fit': {'num_cpus': 4, 'num_gpus': 1}
}
There was a problem hiding this comment.
Here we are intentionally testing when not providing any user specified resources, so the hyperparameter is empty dict. I should just remove hyperparameters
tabular/tests/unittests/resource_allocation/test_total_resource_allocation.py
Outdated
Show resolved
Hide resolved
yinweisu
force-pushed
the
fix_resource_allocation
branch
from
824f926
to
0898da1
Compare
|
Job PR-2536-0898da1 is done. |
|
Job PR-2536-adc4354 is done. |
|
Job PR-2536-fd6a989 is done. |
Issue #, if available:
#2446
Description of changes:
num_resourceandag_args_fitwhen not doing bagging nor hpo. Might think about better way to handle it in the future...By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.