Sacred logger #656

expectopatronum · 2020-01-02T10:15:12Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
Did you read the contributor guideline?
Did you make sure to update the docs? -> I updated the documentation directly in sacred.py. Please let me know if there is documentation somewhere else
Did you write any new necessary tests? -> I wrote similar tests as for other loggers and I am testing/using it in my own project successfully. Please let me know if I should write more tests.

What does this PR do?

Fixes #438.

Did you have fun?

Yes, and I learned a lot :)

Borda

nice work, just add documentation and tests

pytorch_lightning/logging/sacred.py

expectopatronum · 2020-01-02T11:05:12Z

@Borda It's just a draft pull request, I am aware of the things that need to be done ...

CarloLucibello · 2020-01-02T13:37:43Z

If not planned already in this PR, it would be nice for lightning to intercept the Sacred observer created by command line arguments as in

python script.py -F runs/     # FileObserver
python script.py -m dbruns  # MongodbObserver

Not sure how this would work though

expectopatronum · 2020-01-02T13:40:52Z

@CarloLucibello I can think about it but I have only used MongoObserver so far. I also don't really know how the options -F and -m work. I think I'll first make it work with MongoObserver and later extend it (or someone else).

pytorch_lightning/logging/sacred.py

expectopatronum · 2020-01-03T12:29:29Z

EDIT: I think I figured it out, should be easy to use hyperparams like in pytorch lightning and store them in a sacred way 🥳

I am currently a bit unsure how to implement log_hyperparams. As far as I can see, sacred takes care of more stuff than the other loggers that are currently implemented. It does not really allow logging hyperparamters/parameters, since it is designed to take care of that itself. I see a few options:

Using sacred's config and it will take care of logging those parameters itself (but then I probably can't make use of pytorch lightnings grid search anymore)
Use lightnings hparams, log each individually using log_scalar which is not the intended use of this method.
Use lightning hparams, use some other functionality of sacred to log them, e.g. using observer.queued_event like [here](python train_midi_guitar_piano_classifier.py --experiment_name test --batch_size 16)

Maybe I am just making things too complicated but I'd appreciate it if someone had an idea 😅

I found some resources that might go in the right direction (maybe we can use lightning's hparams and pass them to run()):

expectopatronum · 2020-01-05T18:48:20Z

Alright, after I finish implementing the test cases, I think this is ready for review. I have still a few questions left, maybe someone can answer them in the meantime:

Do I need to add a link in README.md?
Do I need to add somewhere else besides in sacred.py?
I always get "The Travis CI build failed" - the only error I can see in the logs is "ImportError: cannot import name 'PILLOW_VERSION'". This seems to be some issue with the installation of Pillow and it seems other current pull requests are also affected. Can/will this be fixed?

Borda · 2020-01-05T19:12:39Z

#661 (comment)

expectopatronum · 2020-01-05T20:41:35Z

#661 (comment)

Ah, I am very sorry! Must have made some mistake, because I was actually searching for an issue before commenting, but I couldn't find it.

expectopatronum · 2020-01-06T07:31:15Z

This pull request is ready imo, but Travis disappeared? I'd like to see if the tests run here before making it "Ready for review".

Borda · 2020-01-06T09:09:28Z

@expectopatronum can you move it from Draft to normal PR status so we can see CI build?

williamFalcon · 2020-01-14T03:16:17Z

@expectopatronum awesome work! @Borda is this ready?

expectopatronum · 2020-01-14T06:05:59Z

@williamFalcon Last time I checked the two tests were failing but I couldn't figure out (didn't have time) why. But it's probably a problem with my tests and not the code itself, because I am using it in my project and everything I expect, works. I don't see Travis now anymore, so I don't know what the status of the tests is.

Borda · 2020-01-14T11:04:54Z

@williamFalcon rebase is needed... and it seems that our master is failing so I would fix the master first...

expectopatronum · 2020-01-15T10:19:22Z

Hi,
at this point I don't know how to proceed. I fixed the conflicts with the base branch as suggested by @Borda. You also mentioned a rebase, is this still necessary after fixing those issues? If yes, please let me know how this should be done.

My test case is working now locally, but Travis still fails with an error related to the TensorboardLogger.

I previously implemented two test functions:

test_sacred_logger (which is fixed now)
test_sacred_pickle (doesn't work and I removed it for now)

The reason for removing it is that I saw that the wandb logger only tests very, very basic stuff.

Maybe you can clarify what functionality needs to be working & tested, since not all loggers seem to be tested the same way.

I am sorry for taking so long to finish this pull request but it's the first time I am working with Travis and at first I wasn't able to reproduce the tests locally (many more were failing bc. pytest didn't take care of the test requirements).

Best regards
Verena

Borda · 2020-01-16T14:21:39Z

@expectopatronum we have fixed master, could you rebase it, pls

- Update __init__ interface - Implement run_id, name, version

- make SacredLogger importable

expectopatronum · 2020-01-18T09:34:41Z

@Borda I did a rebase (took me some time to figure out how that works). Is there a reason why you suggest a rebase rather than merging the changes? (from what I read while figuring out how rebase works, I got the impression merging would make more sense since I already pushed my commits to the remote).

Now it fails with the error ERROR: torchvision 0.4.2 has requirement torch==1.3.1, but you'll have torch 1.4.0 which is incompatible. I guess this is something that has to be fixed in your master?

Borda · 2020-01-18T10:11:17Z

If you do just merge, sometimes the PR diff shows master comits as part of this PR so it is not clear what is the new addition...
Another option is creating new brach from master with the same name, do cherry-picking from the past branch and push it force.. This usually has conflicts to resolve but it is harder to explain =)
Let me see, yesterday the Master was passing with torch 1.4 I check it just I ll get to computer...

expectopatronum · 2020-01-18T10:15:48Z

If you do just merge, sometimes the PR diff shows master comits as part of this PR so it is not clear what is the new addition...

Thanks for the explanation, makes sense :)

williamFalcon · 2020-01-21T13:28:38Z

@expectopatronum rebase master or add sacred to tests/requirements.txt?

Borda

test the logger

Borda · 2020-01-22T10:35:26Z

pytorch_lightning/logging/sacred.py

@@ -0,0 +1,78 @@
+"""
+Log using `sacred <https://sacred.readthedocs.io/en/stable/index.html>'_


pls check the dos visage

Borda · 2020-01-22T12:02:58Z

pytorch_lightning/logging/sacred.py

+    def __init__(self, sacred_experiment):
+        """Initialize a sacred logger.
+
+        :param sacred.experiment.Experiment sacred_experiment: Required. Experiment object with desired observers


use the updated docs formating
:param sacred_experiment (sacred.experiment.Experiment): ...

Borda · 2020-01-22T12:03:39Z

tests/test_logging.py

+    """Verify that basic functionality of sacred logger works."""
+    tutils.reset_seed()
+
+    try:


this should not be here, the logger shall be tested

Borda

tests shall not be tolerant of the missing package...

ozen · 2020-01-24T11:38:48Z

pytorch_lightning/logging/sacred.py

+        return self._run_id
+
+    @rank_zero_only
+    def log_hyperparams(self, params):


I just wanted to share my workflow for your consideration. I use Sacred for general configuration, then Ax for hyperparameter optimization. There is one Sacred experiment and one logger during the whole process, but trainer.fit() is called many times by Ax with different hyperparameters. Logging these in log_hyperparams somehow may be beneficial.

Borda · 2020-01-30T09:52:13Z

by #767 we will do all logger imports at the beginning of the test suit...

williamFalcon · 2020-02-02T14:33:33Z

@expectopatronum hi! mind rebasing and getting the tests to pass? otherwise we should close this PR until it's ready to be worked on

expectopatronum · 2020-02-03T16:42:03Z

Hi, I am sorry, I don't have the time right now and I'm also not using pytorch lightning for my experiments anymore. I hoped to finish this two weeks ago but there are too many changes for me to keep track.

sheiksadique · 2020-07-20T10:34:44Z

Is there still a chance if this PR making it through?

williamFalcon · 2020-07-20T10:38:54Z

mind adding it to bolts?

Borda requested changes Jan 2, 2020

View reviewed changes

pytorch_lightning/logging/sacred.py Outdated Show resolved Hide resolved

pytorch_lightning/logging/sacred.py Outdated Show resolved Hide resolved

pytorch_lightning/logging/sacred.py Show resolved Hide resolved

pytorch_lightning/logging/sacred.py Outdated Show resolved Hide resolved

CarloLucibello reviewed Jan 3, 2020

View reviewed changes

pytorch_lightning/logging/sacred.py Outdated Show resolved Hide resolved

expectopatronum marked this pull request as ready for review January 6, 2020 09:26

expectopatronum added 13 commits January 18, 2020 10:00

Setup sacred_logger

5e1869d

Rename sacred_logger -> sacred

688818c

Add property experiment

62845e1

Setup MongoObserver

2d662c6

Update sacred.py

a4f709f

- Update __init__ interface - Implement run_id, name, version

Update log_metrics to work with sacred

8bceb01

Absolute import

26386e7

Make ImportError more informative

e65defd

Update __init__.py

a63ae05

- make SacredLogger importable

Let user pass list of observers

30eb667

Fix error when accessing name/id

bfd6a80

Let observers default to []

7bbf188

Raise exception in log_hyperparams

64fda62

expectopatronum added 11 commits January 18, 2020 10:02

Add documentation for SacredLogger

6b8b1f3

Update SacredLogger

f6259e7

Update docs/source/conf.py

0cb66bb

Remove optional functions

8bf8402

Add test_sacred_logger

a20f670

Add test_sacred_pickle

c743ea5

Cleanup sacred.py

6b3702c

Fix flake findings

36c72f6

Fix flake8 complaints

e84cbd9

Fix test_sacred_logger

630ddec

Remove test_sacred_pickle

4974847

expectopatronum force-pushed the sacred_logger branch from abbe2ac to 4974847 Compare January 18, 2020 09:07

Update test_logging.py

9fcb238

Borda requested changes Jan 22, 2020

View reviewed changes

Borda requested changes Jan 23, 2020

View reviewed changes

Borda mentioned this pull request Jan 23, 2020

Add sacred logger #438

Closed

ozen reviewed Jan 24, 2020

View reviewed changes

Borda added the feature Is an improvement or enhancement label Jan 31, 2020

Borda added this to the 0.6.1 milestone Jan 31, 2020

expectopatronum closed this Feb 3, 2020

akihironitta mentioned this pull request Dec 12, 2020

Add a logger for Sacred Lightning-Universe/lightning-bolts#443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sacred logger #656

Sacred logger #656

expectopatronum commented Jan 2, 2020 •

edited

Loading

Borda left a comment

expectopatronum commented Jan 2, 2020

CarloLucibello commented Jan 2, 2020

expectopatronum commented Jan 2, 2020

expectopatronum commented Jan 3, 2020 •

edited

Loading

expectopatronum commented Jan 5, 2020

Borda commented Jan 5, 2020

expectopatronum commented Jan 5, 2020

expectopatronum commented Jan 6, 2020

Borda commented Jan 6, 2020

williamFalcon commented Jan 14, 2020

expectopatronum commented Jan 14, 2020

Borda commented Jan 14, 2020

expectopatronum commented Jan 15, 2020

Borda commented Jan 16, 2020

expectopatronum commented Jan 18, 2020

Borda commented Jan 18, 2020

expectopatronum commented Jan 18, 2020 •

edited by Borda

Loading

williamFalcon commented Jan 21, 2020

Borda left a comment

Borda Jan 22, 2020

Borda Jan 22, 2020

Borda Jan 22, 2020

Borda left a comment

ozen Jan 24, 2020 •

edited

Loading

Borda commented Jan 30, 2020

williamFalcon commented Feb 2, 2020

expectopatronum commented Feb 3, 2020

sheiksadique commented Jul 20, 2020

williamFalcon commented Jul 20, 2020

		@@ -0,0 +1,78 @@
		"""
		Log using `sacred <https://sacred.readthedocs.io/en/stable/index.html>'_

Sacred logger #656

Sacred logger #656

Conversation

expectopatronum commented Jan 2, 2020 • edited Loading

Before submitting

What does this PR do?

Did you have fun?

Borda left a comment

Choose a reason for hiding this comment

expectopatronum commented Jan 2, 2020

CarloLucibello commented Jan 2, 2020

expectopatronum commented Jan 2, 2020

expectopatronum commented Jan 3, 2020 • edited Loading

expectopatronum commented Jan 5, 2020

Borda commented Jan 5, 2020

expectopatronum commented Jan 5, 2020

expectopatronum commented Jan 6, 2020

Borda commented Jan 6, 2020

williamFalcon commented Jan 14, 2020

expectopatronum commented Jan 14, 2020

Borda commented Jan 14, 2020

expectopatronum commented Jan 15, 2020

Borda commented Jan 16, 2020

expectopatronum commented Jan 18, 2020

Borda commented Jan 18, 2020

expectopatronum commented Jan 18, 2020 • edited by Borda Loading

williamFalcon commented Jan 21, 2020

Borda left a comment

Choose a reason for hiding this comment

Borda Jan 22, 2020

Choose a reason for hiding this comment

Borda Jan 22, 2020

Choose a reason for hiding this comment

Borda Jan 22, 2020

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

ozen Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Borda commented Jan 30, 2020

williamFalcon commented Feb 2, 2020

expectopatronum commented Feb 3, 2020

sheiksadique commented Jul 20, 2020

williamFalcon commented Jul 20, 2020

expectopatronum commented Jan 2, 2020 •

edited

Loading

expectopatronum commented Jan 3, 2020 •

edited

Loading

expectopatronum commented Jan 18, 2020 •

edited by Borda

Loading

ozen Jan 24, 2020 •

edited

Loading