Comet fix #481
Comet fix #481
Conversation
|
This is my first attempt at a PR so I'm not sure why the CI check is failing. It seems to be a failure when installing dependencies, but I haven't altered the requirements.txt file. |
| super(CometLogger, self).__init__() | ||
| self.experiment = CometExperiment(*args, **kwargs) | ||
| self.experiment = CometExperiment(api_key=api_key, workspace=workspace, project_name=project_name, *args, |
There was a problem hiding this comment.
You didn't introduce this problem, but maybe you can fix it while you're working on this class. We need to create the experiment lazily so that we don't get duplicates when running in distributed mode. If you're willing to take a stab at fixing this, take a look at one of the other loggers to see how this is done.
There was a problem hiding this comment.
Hi Nic, thanks for the tips. I've just pushed a version of the code where I attempt to create the experiment lazily, but it's a bit outside my comfort zone at the moment so please let me know if there's a way I could improve the code.
Additionally, I've noticed that the Travis-ci testing has failed the same way for all PRs in the last 24 hours or so. Is that something that can be addressed easily?
| nb_exps = len(self.comet_api.get_experiments(self.workspace, self.project_name)) | ||
| return nb_exps + 1 | ||
| else: | ||
| return None |
There was a problem hiding this comment.
not sure if returning None as a version is a good idea
There was a problem hiding this comment.
From what I can tell, logger.version is only being used in the tqdm progress bar. If version is None, it is ignored, otherwise the version is displayed in the progress bar.
If there are future plans to use this version elsewhere, I'm open to suggestions for a different default return value.
@Borda This part I might need some help with. Since Comet.ml does logging remotely, the tests likely won't resemble the other logger tests. Should we set up a dummy account on Comet.ml so we can test a real connection? |
|
Other than that one minor comment about changing the type of exception, looks good to me, assuming the tests pass. |
|
@rwesterman if you feel it is ready, drop WIP from the PR name :) |
|
@williamFalcon I believe that it is ready... |
Before submitting
What does this PR do?
Fixes #470
I've fixed the NotImplementedError for the name() and version() class methods. I've also added the following functionality: