Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add out_dir option to eval #244

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

alexander-soare
Copy link
Collaborator

@alexander-soare alexander-soare commented Jun 4, 2024

What this does

Adds the ability to specify a custom output directory for the eval.py script.

Side changes:

  • Change def eval to def main to avoid clash with native python eval (because it's good practice and makes it easier to read the code)
  • Change the video directory to be called "video" instead of "eval" (more precise. I actually got confused for a moment and though I implemented the output directory incorrectly)

How it was tested

CI

How to checkout & try? (for the reviewer)

To check that the custom output directory works:

python lerobot/scripts/eval.py -p lerobot/diffusion_pusht --out-dir example_eval eval.n_episodes=1 eval.batch_size=1 +policy.noise_scheduler_type=DDIM policy.num_inference_steps=10

To check that the default works:

python lerobot/scripts/eval.py -p lerobot/diffusion_pusht eval.n_episodes=1 eval.batch_size=1 +policy.noise_scheduler_type=DDIM policy.num_inference_steps=10


This change is Reviewable

@Cadene
Copy link
Collaborator

Cadene commented Jun 4, 2024

Did you search in the code base for eval.py and eval and update the documentation if any? Thanks!

Also it's a bit weird that the unit tests didnt fail if the video_dir changed: video_dir=Path(out_dir) / "video"

@alexander-soare
Copy link
Collaborator Author

alexander-soare commented Jun 4, 2024

@Cadene yeah I did a search. eval on its own brings up a lot of hits so I did what I could. Which of the tests do you think should fail? I don't think they check for the video directory specifically.

@Cadene
Copy link
Collaborator

Cadene commented Jun 4, 2024

@Cadene yeah I did a search. eval on its own brings up a lot of hits so I did what I could. Which of the tests do you think should fail? I don't think they check for the video directory specifically.

I was thinking https://github.com/huggingface/lerobot/blob/main/examples/4_train_policy_with_script.md
since it write a video during evaluation in a certain directory (eval in the experiment directory).

Copy link
Collaborator

@Cadene Cadene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ;) Ping me for second review

lerobot/scripts/eval.py Outdated Show resolved Hide resolved
lerobot/scripts/eval.py Show resolved Hide resolved
@@ -332,7 +332,7 @@ def evaluate_and_checkpoint_if_needed(step):
eval_env,
policy,
cfg.eval.n_episodes,
video_dir=Path(out_dir) / "eval",
video_dir=Path(out_dir) / "video",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

Suggested change
video_dir=Path(out_dir) / "video",
video_dir=Path(out_dir) / "eval" / "videos_step_{step:09d}",

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. PTAL at my changes.

@@ -546,7 +543,7 @@ def eval(
policy,
hydra_cfg.eval.n_episodes,
max_episodes_rendered=10,
video_dir=Path(out_dir) / "eval",
video_dir=Path(out_dir) / "video",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

Suggested change
video_dir=Path(out_dir) / "video",
videos_dir=Path(out_dir) / "videos",

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with that. Done.

Copy link
Collaborator

@Cadene Cadene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO without tests

@Cadene Cadene merged commit 21f222f into huggingface:main Jun 4, 2024
5 checks passed
@tlpss
Copy link
Contributor

tlpss commented Jun 4, 2024

FYI, I had to reinstall the package (even though it was installed in editable mode) to overcome this error:

Error executing job with overrides: ['+experiment=pusht-act']
Traceback (most recent call last):
  File "/home/tlips/Code/lerobot/lerobot/scripts/train.py", line 424, in train_cli
    train(
  File "/home/tlips/Code/lerobot/lerobot/scripts/train.py", line 414, in train
    evaluate_and_checkpoint_if_needed(step + 1)
  File "/home/tlips/Code/lerobot/lerobot/scripts/train.py", line 337, in evaluate_and_checkpoint_if_needed
    eval_info = eval_policy(
TypeError: eval_policy() got an unexpected keyword argument 'videos_dir'

Seems to work fine now though!

PS. You forgot to update the docstring here

@alexander-soare
Copy link
Collaborator Author

@tlpss thanks a lot for pointing that out! Ugh I knew I'd miss something...

@alexander-soare alexander-soare deleted the custom_eval_out_dir branch June 5, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants