fix: Autorom manual download. #300
Conversation
KaleabTessera
requested review from
arnupretorius and
DriesSmit
as code owners
There was a problem hiding this comment.
This looks great! Thanks @KaleabTessera 👍 I really like the lp_utils.cpu_only(program) code. Can you maybe do the same for the examples? Something like lp_utils.gpu_trainer_only(program) should probably work.
mava/utils/lp_utils.py
Outdated
|
|
||
| FLAGS = flags.FLAGS | ||
|
|
||
|
|
||
| def cpu_only(program: Any) -> Dict: |
We could also perhaps think about making a |
|
@DriesSmit I am still adding the example conversions :) |
Thanks @KaleabTessera 👍 🙌 |
| } | ||
| local_resources = lp_utils.to_gpu( | ||
| program_nodes=program.groups.keys(), nodes_on_gpu=["trainer"] | ||
| ) |
There was a problem hiding this comment.
This looks great 🙌 🔥 Thanks @KaleabTessera!
There was a problem hiding this comment.
Nice @KaleabTessera!! 😄 Much cleaner than before! Just see my few comments.
|
|
||
| Args: | ||
| program_nodes (List): nodes in lp program. | ||
| nodes_on_gpu (List, optional): nodes to run on gpu. Defaults to ["trainer"]. | ||
|
|
||
| Returns: | ||
| Dict: dict with cpu only lp config. |
tests/systems/dial_system_test.py
Outdated
| @@ -65,7 +65,7 @@ def test_recurrent_dial_on_debugging_env(self) -> None: | |||
| trainer_node.disable_run() | |||
|
|
|||
| # Launch gpu config - don't use gpu | |||
| local_resources = lp_utils.cpu_only(program) | |||
| local_resources = lp_utils.cpu_only(program.groups.keys()) | |||
There was a problem hiding this comment.
I know I suggested feeding in the list, but with the nodes_on_gpu list as an additional argument (specifying the subset of the full list), I think it is cleaner to feed in only the program and keep the for node in program.groups.keys() inside the to_gpu. 😄 That way, the cpu_only looks nice and clean as before. Also, one suggestion is perhaps to have a single util function called to_device which defaults to all cpu if the nodes_on_gpu is empty. What do you think @KaleabTessera? Happy either way, since the former is more clear when reading the code while the latter is more compact.
There was a problem hiding this comment.
I think having cpu_only and to_gpu taking in a list might be the better option, mainly because it is easier to test. We don't have to initialize a program to test the functions works. From my perspective, passing program or program.groups.keys() seems quite similar in terms of cleaness.
For the to_device function, it sounds like a good idea. I will update the code :)
What?
Autorom temp fix.
Why?
How?
Manual install atari roms.
Extra