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

Log locally with tensorboard when wandb is disabled. #154

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dirkmcpherson
Copy link

@dirkmcpherson dirkmcpherson commented May 8, 2024

What this does

This PR adds support for locally logging metrics and videos using tensorboard when wandb is disabled.

How it was tested

  • Ran with a short eval_freq and log_freq to ensure metric and video logging worked through multiple periods.
  • Tested with tensorboard.active = true / false and wandb.active = true / false
  • ran pytests (passed)

How to checkout & try? (for the reviewer)

Run with wandb.enable=false and short eval / log freqs. e.g.
python lerobot/scripts/train.py policy=diffusion env=pusht env.task=PushT-v0 dataset_repo_id=lerobot/pusht training.log_freq=10 training.eval_freq=100 wandb.enable=false tensorboard.enable=true


This change is Reviewable

@dirkmcpherson dirkmcpherson changed the title Log locally with tensorbaord when wandb is disabled. Log locally with tensorboard when wandb is disabled. May 8, 2024
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, we should probably add option for tensorboard logging as it is quite used.
Anyone knows what is the usage for these kind of logging/viz tools in the community?
Is wandb more popular than tensorboard? What are the alternatives?

@@ -349,8 +349,7 @@ def evaluate_and_checkpoint_if_needed(step):
start_seed=cfg.seed,
)
log_eval_info(logger, eval_info["aggregated"], step, cfg, offline_dataset, is_offline)
if cfg.wandb.enable:
logger.log_video(eval_info["video_paths"][0], step, mode="eval")
logger.log_video(eval_info["video_paths"][0], step, mode="eval")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not add a cfg.tensorboard.enable argument to support both?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea! Will do.

@aliberts aliberts added the ✨ Enhancement New feature or request label May 12, 2024
@kashyapakshay
Copy link
Contributor

kashyapakshay commented May 14, 2024

Thanks, we should probably add option for tensorboard logging as it is quite used. Anyone knows what is the usage for these kind of logging/viz tools in the community? Is wandb more popular than tensorboard? What are the alternatives?

Both are quite popular and could be preferable in different scenarios I think. A lot of people prefer Tensorboard (including myself) because of how little overhead there is to spin up and start using it, especially when you just want to visualize loss curves and maybe some input/output samples. Even some teams who don't mind managing some infra use it (my previous ML team had Tensorboard instances hosted on K8s backed by S3 storage for artifacts). Wandb of course is a managed platform with more sophisticated workflows around tracking, hosting, sharing results, etc., but comes at a cost (although the free plan is pretty generous for small-scale experimentation). In any case, think it would be great to have Tensorboard support!

@dirkmcpherson dirkmcpherson requested a review from Cadene May 17, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants