Comet #215
Comet #215
Conversation
There was a problem hiding this comment.
That's for the PR! I requested a few inline changes. A few additional comments:
- In the
setup.pyfile, you should add an optional dependency forcometand also modifyextras["all"]andextras["dev"]. Let me know if you have any questions about this. - In the
scriptsfolder there are four files which run experiments. Could you add writer as an additional argument? - Normally I would insist on unit tests, but the tensorboard writer is also lacking them due to the need to mock the underlying objects. Will have to revisit this at some point.
| @@ -2,6 +2,7 @@ | |||
| import numpy as np | |||
| from scipy import stats | |||
| from .writer import ExperimentWriter | |||
| from .writer import CometWriter | |||
There was a problem hiding this comment.
You can use one line to import both of these, i.e. from .writer import ExperimentWriter, CometWriter
| def _make_writer(self, logdir, agent_name, env_name, write_loss, writer): | ||
| if writer == "comet": | ||
| return CometWriter(self, agent_name, env_name, loss=write_loss, logdir=logdir) | ||
| else: |
There was a problem hiding this comment.
It is preferred not to have this else because the if block contains a return.
| def _make_writer(self, logdir, agent_name, env_name, write_loss, writer): | ||
| if writer == "comet": | ||
| return CometWriter(self, agent_name, env_name, loss=write_loss, logdir=logdir) | ||
| else: |
all/experiments/writer.py
Outdated
| @@ -5,11 +5,18 @@ | |||
| from datetime import datetime | |||
| from tensorboardX import SummaryWriter | |||
| from all.logging import Writer | |||
| from comet_ml import Experiment | |||
There was a problem hiding this comment.
This import fails if comet isn't installed. That appears to by why the tests are failing.
all/experiments/writer.py
Outdated
| try: | ||
| from comet_ml import Experiment | ||
| except ImportError: | ||
| COMET_AVAILABLE = False |
There was a problem hiding this comment.
It would probably be better to include this block inside the constructor for CometWriter.
all/experiments/writer.py
Outdated
| self.add_evaluation(name + "/std", std, step) | ||
|
|
||
| def add_scalar(self, name, value, step="frame"): | ||
| pass |
There was a problem hiding this comment.
Not ideal to have this silently fail.
all/logging/__init__.py
Outdated
| # @abstractmethod | ||
| # def set_phase(self, name, mean, std, step="frame"): | ||
| # # TODO: Fill | ||
| # pass |
| # pass | ||
|
|
||
| @abstractmethod | ||
| def close(self): |
There was a problem hiding this comment.
This abstract method was added, but not used anywhere. Should run_experiment call this somehow?
There was a problem hiding this comment.
This is actually called in the Experiment save method. The comet writer doesn't know when to stop an experiment in its dashboard unless you run close or the script ends. This is an issue in jupyter notebooks running interactively because comet won't close the experiment until you start a new one or shut down jupyter. Let me know if you think this is an issue or have any better ways of handling it.
There was a problem hiding this comment.
I don't think it should be in save because you might want to resume the experiment after. Perhaps the right fix is to add an experiment.close() method.
|
Unrelated the code itself: If in the PR you enter the issue number (i.e. #214 ) the project page will magically advance the note. |
all/experiments/writer.py
Outdated
|
|
||
| self._comet.set_name(agent_name) | ||
| self.log_dir = logdir | ||
| # super().__init__(log_dir=self.log_dir) |
Added comet writer and made necessary changes to use it with run_experiment().
#214