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

fixing TensorBoard #687

Merged
merged 9 commits into from
Jan 16, 2020
Merged

fixing TensorBoard #687

merged 9 commits into from
Jan 16, 2020

Conversation

Borda
Copy link
Member

@Borda Borda commented Jan 15, 2020

What does this PR do?

After a couple last merges the master is failing again, so this is an attempt to get it back on track...
in particular, the replacement of Test-tube by buildin TnesorBorard was not ready... #609

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

@Borda
Copy link
Member Author

Borda commented Jan 15, 2020

fighting with while installing the package...

Traceback (most recent call last):
  File "/home/travis/build/Borda/pytorch-lightning/vEnv/lib/python3.7/site-packages/torch/utils/tensorboard/__init__.py", line 2, in <module>
    from tensorboard.summary.writer.record_writer import RecordWriter  # noqa F401
ModuleNotFoundError: No module named 'tensorboard'

see https://travis-ci.org/Borda/pytorch-lightning/jobs/637518732#L838
similar to

@Borda
Copy link
Member Author

Borda commented Jan 15, 2020

@PyTorchLightning/core-contributors ^^

@Borda Borda added the bug Something isn't working label Jan 15, 2020
@Borda Borda self-assigned this Jan 15, 2020
@Borda Borda added this to the 0.6.0 milestone Jan 15, 2020
# save the metatags file
df = pd.DataFrame({'key': list(self.tags.keys()),
'value': list(self.tags.values())})
df.to_csv(meta_tags_path, index=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't all this tag saving stuff go into the log_hyperparams function, not log_metrics?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question, what do you propose? @williamFalcon

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just move all this new code up into where the warning is in log_hyperparams.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed with @neggert

Copy link
Member Author

Choose a reason for hiding this comment

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

so you want to do a save always when a new parameter is added?
it makes more sense for me to do it while saving...

Copy link
Contributor

Choose a reason for hiding this comment

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

log_metrics gets called all the time. we shouldn’t save the csv over and over when that happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

so saving it in save() is fine? or am I missing something...

Copy link
Contributor

Choose a reason for hiding this comment

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

i haven’t looked at this in a while. i would just put it where test tube does it

Copy link
Member Author

Choose a reason for hiding this comment

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

test-tube does it while saving too, that's how I "copy-paste" it...
https://github.com/williamFalcon/test-tube/blob/master/test_tube/log.py#L366

Copy link
Contributor

Choose a reason for hiding this comment

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

log_hyperparams is intended to be called once at the beginning of training. I think that's what we want.

# try if the new sub-folder exists, typical case for test-tube
if not os.path.isdir(path_expt):
path_expt = path_dir
return path_expt
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of special cases for test tube here. Fine for now to get the build passing, but let's revisit later and see if we can avoid special cases.

Copy link
Member Author

@Borda Borda Jan 15, 2020

Choose a reason for hiding this comment

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

agree, this was just a side-work when I was updating paths/links and found that the master fails... shall we make a ticket for it so we won't forget?

@williamFalcon
Copy link
Contributor

williamFalcon commented Jan 15, 2020

@Borda
lgtm except for the moving to log_hyperparams

@Borda
Copy link
Member Author

Borda commented Jan 15, 2020

have you met the following issue?

    from torch.jit.annotations import Optional
E   ImportError: cannot import name 'Optional'

https://app.circleci.com/jobs/github/Borda/pytorch-lightning/839

This was referenced Jan 16, 2020
@Borda
Copy link
Member Author

Borda commented Jan 16, 2020

@williamFalcon @neggert ready to go...

@williamFalcon williamFalcon merged commit f72e354 into Lightning-AI:master Jan 16, 2020
@williamFalcon
Copy link
Contributor

@Borda let's make a different PR to make sure hparams don't save on every log metrics

@Borda Borda deleted the tensorboard branch January 16, 2020 12:38
williamFalcon added a commit that referenced this pull request Jan 17, 2020
* updated gitignore

* Update README.md

* updated gitignore

* updated links in ninja file

* updated docs

* Update README.md

* Update README.md

* finished callbacks

* finished callbacks

* finished callbacks

* fixed left menu

* added callbacks to menu

* added direct links to docs

* added direct links to docs

* added direct links to docs

* added direct links to docs

* added direct links to docs

* fixing TensorBoard (#687)

* flake8

* fix typo

* fix tensorboardlogger
drop test_tube dependence

* formatting

* fix tensorboard & tests

* upgrade Tensorboard

* test formatting separately

* try to fix JIT issue

* add tests for 1.4

* added direct links to docs

* updated gitignore

* updated links in ninja file

* updated docs

* finished callbacks

* finished callbacks

* finished callbacks

* fixed left menu

* added callbacks to menu

* added direct links to docs

* added direct links to docs

* added direct links to docs

* added direct links to docs

* added direct links to docs

* added direct links to docs

* finished rebase

* making private  members

* making private  members

* making private  members

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* set auto dp if no backend

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* working on trainer docs

* fixed lightning import

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* cleared  spaces

* finished lightning module

* finished lightning module

* finished lightning module

* finished lightning module

* added callbacks

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* set auto dp if no backend

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* added loggers

* flake 8

* flake 8

Co-authored-by: Jirka Borovec <[email protected]>
@Borda Borda mentioned this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants