Closed timokau closed 1 year ago
I'm inclined to say this is indeed too big a change for us to maintain long-term, especially since we don't have any internal users for other experiment trackers. It's unlikely we'll accept this PR, but I will raise internally for discussion. @zplizzi any thoughts?
Out of interest, what are the other tracker backend(s) you are looking to use @timokau ?
I'm not entirely opposed to this, having a layer of abstraction around wandb doesn't cost much in terms of maintenance and definitely helps people that use other trackers. The biggest technical question would be whether to leave the abstraction as a global object (which is nice because you can call it from anywhere without having to pass around a tracker object everywhere, which gets tedious), or making it a local object (which seems to be what was done here, and has its own benefits). I'd lean towards leaving it as a global - this is how other logging frameworks work, too.
@timokau we chatted about this and if you're willing to switch this over to a global-style logging pattern, I'd be happy to help you merge it in! Unfortunately I just did some refactoring (making things faster!) that probably generated some merge conflicts, but should be easy to fix. But also no pressure if you don't want to keep working on this.
Hi, sorry for the late response. Thanks a lot for considering this! Unfortunately I won't have time for a major rework of this, at least in the short term :/
Why do you think a global logger is better than a local one? The passing around seems reasonable to me here and having a global object would again make it harder to cleanly exchange the logger.
No worries, I'll close this then but we can pick it up in the future if desired. I think global just matches the norm of other logging frameworks that we use - python's builtin logging
, loguru
, wandb
, etc.
This PR makes it optional to use wandb and instead allows users to configure their own experiment tracking framework. The comment from #12 applies here as well: I realize that this is a very big change and you may not want to merge this. I worked on this for my own needs and I understand if you consider this out of scope. I do think that this makes the baselines much more reusable however, which would be nice since this is one of the best dreamer implementations that I could find.
The intention of this PR is to start a discussion. If you are in principle willing to make the experiment tracker configurable, I'm happy to continue to work on this and iron out the remaining issues.
This is not mergeable in its current state (see remaining issues below), but can be used if someone wants to use avalon baseline agents without wandb.
Remaining issues:
avalon.agent.torchbeast.polybeast_learner.run_test depends on wandb.Video through encode_video (it's only usage) and on wandb.Image, but is never called
similar for avalon.agent.godot.godot_eval
avalon.agent.torchbeast.polybeast still depends on wandb, mostly to manage checkpoints. It should be possible to replace that with a ExperimentTracker reasonably easily, but I did not touch it for now since I am unfamiliar with that code.
a comment stated that "[wandb_init] needs to happen after all other processes launch". This is not the case anymore, since we need to pass a tracker to
algorithm
and we need to initializealgorithm
before initializing the rollout manager. I am not sure why wandb_init should only be called after all other processes launch.