Hyperparameter Optimization Module #164

middleyuan · 2024-09-23T14:42:08Z

This PR is created for two primary purposes:

Include the state-of-the-art package Google-Vizier as a hyperparameter optimization solver.
Change the database to SQLite for easier usage.

…ve unnecessary config in cartpole_stab.yaml 2. add hpo module in test_build.py

… of JSRL on PPO.

…ckpoint in ppo.py. 4. Boolean var in ppo_sampler.

…dd an example of hpo for gpmpc.

… avg return in base_experiment.py. 3. use BaseExperiment class in hpo example 3. add hp study bash script and jupyter notebook for gpmpc.

… config. 3. add done_on_max_steps in base_experiment.py. 4. remove _run() and use BaseExperiment in hpo.

…SQLite. 4. Unittest for HPO for iLQR, PPO, and GPMPC on cartpole.

adamhall

Looks pretty good! I have a couple thoughts though:

It seems like there is a lot of controller-specific code. For example, there is a separate sampler for each controller, and in the HPO code, there are if statements depending on the algorithm being optimized. I'm wondering if there is a way to make this more generic by making an HPO yaml more complex and then having the underlying code make classes from the arguments in these yamls? For example, I think that hpo_sampler.py could almost entirely be defined in a yaml and then having a generic class for sampler that parses the yaml appropriately? It feels like there is a lot of repeated code that could be simplified and the addition of future algos simpler?
the HPO class is defined in both hpo_optuna.py and hpo_vizier.py which seem to share a lot of code. I think there should really be a parent HPO class, and then child sublcasses for the different use cases?
I'm not totally sure why the files are being removed from the examples. Is it because you are replacing them with better hyper parameters?

adamhall · 2024-09-23T16:18:10Z

safe_control_gym/hyperparameters/hpo_optuna.py

+
+class HPO(object):
+
+    def __init__(self, algo, task, load_study, output_dir, task_config, hpo_config, algo_config, safety_filter=None, sf_config=None):


Put each argument on a separate line and provide types and defaults for all if possible.

safe_control_gym/hyperparameters/hpo_optuna.py

adamhall · 2024-09-23T16:41:07Z

safe_control_gym/hyperparameters/hpo_optuna.py

+                Gs = np.inf
+            for i in range(self.hpo_config.repetitions):
+                # np.random.seed()
+                seed = np.random.randint(0, 10000)


We should be careful about this for reproducibility. In the envs, we usually create a random seeding object that is used for all randomness. Can we incorporate this somehow to ensure everything is exactly reproducible?

Yes, see hpo_experiment.py.

adamhall · 2024-09-23T16:57:22Z

safe_control_gym/hyperparameters/hpo_optuna.py

+                seed = np.random.randint(0, 10000)
+                # update the agent config with sample candidate hyperparameters
+                # new agent with the new hps
+                for hp in sampled_hyperparams:


To make this more generic, is there a way to make the keys part of the sampled_hyperparams dictionary? Could hp object also include the name like 'q_mpc', 'r_mpc'? Otherwise, this logic has to be updated whenever a new controller is added.

adamhall · 2024-09-23T17:21:35Z

safe_control_gym/hyperparameters/hpo_vizier.py

+from safe_control_gym.utils.registration import make
+from safe_control_gym.utils.utils import mkdirs
+
+class HPO(object):


This class is defined in both hpo_optuna and hpo_vizier, naming should be more precise instead of having the same class defined in two places.

Federico-PizarroBejarano

I agree with Adam's comments and believe they should be addressed before the next round of reviews. The changes are too broad to do only one review. In general I think there is too much code, both repeated and non-repeated. I think the gym needs to generally simple and concise for people (other than us) to use it. There is also a lot of commented out code which I don't think should be there in completed code. Also, two stylistic comments: the precommit hooks need to be run to format everything consistently, and the docstrings and comments should follow our standard style, as Adam pointed out.

Federico-PizarroBejarano · 2024-09-24T13:03:51Z

examples/hpo/cartpole/config_overrides/gp_mpc_cartpole_stab_200.yaml

-device: cpu
-restore: null
+device: cuda
+restore: null


there should be a newline at the end of the file. I think the pre-commit hooks handle that. If not, you can configure vs code to always add one when a file is saved, which is what I do

Yeah the pre-commit hook should fix this, which means you haven't yet run the hook. This should be done ASAP as it may change a lot

I think the hooks have still not been run, since there are double quotes here and there.

examples/hpo/cartpole/config_overrides/ppo_cartpole_stab_.yaml

examples/hpo/hpo_experiment.py

adamhall · 2024-09-27T19:14:58Z

Looks much cleaner! Nice! I think the samplers could still be made a little more flexible? Let me know if you think this is feasible.

middleyuan · 2024-09-30T12:25:42Z

Looks pretty good! I have a couple thoughts though:

It seems like there is a lot of controller-specific code. For example, there is a separate sampler for each controller, and in the HPO code, there are if statements depending on the algorithm being optimized. I'm wondering if there is a way to make this more generic by making an HPO yaml more complex and then having the underlying code make classes from the arguments in these yamls? For example, I think that hpo_sampler.py could almost entirely be defined in a yaml and then having a generic class for sampler that parses the yaml appropriately? It feels like there is a lot of repeated code that could be simplified and the addition of future algos simpler?

the HPO class is defined in both hpo_optuna.py and hpo_vizier.py which seem to share a lot of code. I think there should really be a parent HPO class, and then child sublcasses for the different use cases?

I'm not totally sure why the files are being removed from the examples. Is it because you are replacing them with better hyper parameters?

I have made the HPO code more generic. The reason I don't define hyperparameter search space in yaml is that I don't want to add burdens on users as it usually requires some knowledge for code and algorithms.
Yes, corresponding changes are made.
Changes include re-factoring to make the folder structure consistent with other examples.

General comments: to get HPO module fully tested, I am waiting for another PR (quadrotor interface) to be approved. After that I will run unit-test for HPO on new env interface and also run pre-commits hook.

…the unit tests pass.

middleyuan added 30 commits June 30, 2023 18:29

1. bug fixed. 2. kernel extension. 3. batch GP implementatoin.

8d3be23

update dependencies

7f0e3ff

explicitliy import scipy.linalg

cf3d4e8

add cartpole configs for gpmpc

359eecc

add hyperparameter optimization module

89a29b8

catch all the exception in hpo for debugging purpose.

9e2a7ef

put cartpole configs for gpmpc under the folder of gpmpc

27454da

add hpo scripts

17e408b

1. include pandas 2. change rel import in gpmpc_experiment.py 3. remo…

bb4e1b0

…ve unnecessary config in cartpole_stab.yaml 2. add hpo module in test_build.py

rename config to match default algo name.

14a6db1

remove old configs

e6a2e3d

add tests

0484388

edit bash file with correct arg name

84830df

add another host in gpmpc_hpo.sh

e69e048

change to new dir in gpmpc_hpo.sh

097e1c2

1. fix a small bug 2. add test_train_gpmpc_cartpole

405dcea

add a hpo parallelism test

549ff3e

saving before runing hpo

81b5602

I think the bug is that it reaches thee goal in the first step.

a5ad5f2

1. PPO configs. 2. Make cartpole init states harder. 3. First version…

ce4d75e

… of JSRL on PPO.

Re-organize a bit (file name, remove __init__.py in test folders).

b40566c

1. HPO strategies. 2. test on hpo for ppo. 3. another way to save che…

23f571d

…ckpoint in ppo.py. 4. Boolean var in ppo_sampler.

update gitignore

802edb6

change configs

02d1c33

update bash for hpo on gpmpc

20d3a7f

add prior arg in gpmpc_sampler

ad96f6f

1. HPO effort evaluations. 2. Bash file for hpo strategy evalution.

5318c25

update dependencies

924d3b3

add the freedom to choose between random sampler and TPE sampler.

14ae2aa

1. add strategy 5. 2. add unit test accordingly.

c0b1b34

middleyuan added 13 commits April 17, 2024 11:57

1. standardize hpo template in the examples. 2. remove _learn(). 3. a…

ba837c5

…dd an example of hpo for gpmpc.

run pre-commit hooks.

51c601e

add gpmpc hpo test without using mysql

fb572f2

1. update config of cartpole task. 2. add max_steps and exponentiated…

c5fbeed

… avg return in base_experiment.py. 3. use BaseExperiment class in hpo example 3. add hp study bash script and jupyter notebook for gpmpc.

1. add bash files to automate hpo pipeline for gpmpc. 2. update gpmpc…

f8e3d0c

… config. 3. add done_on_max_steps in base_experiment.py. 4. remove _run() and use BaseExperiment in hpo.

Merge remote-tracking branch 'upstream/main' into hpo

1f62d3b

match .gitignore to upstram/main.

ffb29de

update for review

1508634

update based on the review comments.

896ac9f

fix typo in readme.

3b49f7b

make test_train.py faster.

133dfee

Merge remote-tracking branch 'utiasDSL/main' into hpo

fc6fd06

1. add Vizier package. 2. update dependencies. 3. change database to …

2cf9424

…SQLite. 4. Unittest for HPO for iLQR, PPO, and GPMPC on cartpole.

middleyuan added the enhancement New feature or request label Sep 23, 2024

middleyuan requested review from adamhall and Federico-PizarroBejarano September 23, 2024 14:42

update readme.

1828ff1

adamhall requested changes Sep 23, 2024

View reviewed changes

Federico-PizarroBejarano requested changes Sep 24, 2024

View reviewed changes

middleyuan self-assigned this Sep 26, 2024

commit for the second round review.

265d8df

1. improve docstrings. 2. add a metric for hpo. 3. add hpo.sh bash.

32596c8

middleyuan added 2 commits October 1, 2024 15:06

1. some bugs fixed. 2. add special handling. 3. make tests faster.

adbbb46

1. add hpo objective bounds in config. 2. improve logging.py. 3. all …

f89197a

…the unit tests pass.

middleyuan requested review from adamhall and Federico-PizarroBejarano October 2, 2024 09:59

middleyuan added 2 commits October 2, 2024 12:04

fix minor config issue.

2c046ef

some bug fix.

67cb7e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hyperparameter Optimization Module #164

Hyperparameter Optimization Module #164

middleyuan commented Sep 23, 2024 •

edited

Loading

adamhall left a comment

adamhall Sep 23, 2024

adamhall Sep 23, 2024

middleyuan Oct 2, 2024

adamhall Sep 23, 2024

middleyuan Oct 2, 2024

adamhall Sep 23, 2024

middleyuan Oct 2, 2024

Federico-PizarroBejarano left a comment

Federico-PizarroBejarano Sep 24, 2024

Federico-PizarroBejarano Sep 24, 2024

middleyuan Oct 2, 2024

Federico-PizarroBejarano Oct 8, 2024

adamhall commented Sep 27, 2024 •

edited

Loading

middleyuan commented Sep 30, 2024 •

edited

Loading


		class HPO(object):

		def __init__(self, algo, task, load_study, output_dir, task_config, hpo_config, algo_config, safety_filter=None, sf_config=None):

Hyperparameter Optimization Module #164

Are you sure you want to change the base?

Hyperparameter Optimization Module #164

Conversation

middleyuan commented Sep 23, 2024 • edited Loading

adamhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Federico-PizarroBejarano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamhall commented Sep 27, 2024 • edited Loading

middleyuan commented Sep 30, 2024 • edited Loading

middleyuan commented Sep 23, 2024 •

edited

Loading

adamhall commented Sep 27, 2024 •

edited

Loading

middleyuan commented Sep 30, 2024 •

edited

Loading