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

LoggerCollection breaks PyTorch profiler #8157

Closed
gahdritz opened this issue Jun 27, 2021 · 3 comments · Fixed by #8187
Closed

LoggerCollection breaks PyTorch profiler #8157

gahdritz opened this issue Jun 27, 2021 · 3 comments · Fixed by #8187
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Open to be worked on priority: 0 High priority task
Milestone

Comments

@gahdritz
Copy link
Contributor

gahdritz commented Jun 27, 2021

🐛 Bug

If the Trainer's profiler parameter is set to "pytorch" and the Trainer's logger is an instance of LoggerCollection, the profiler fails to write to a local file (with a warning).

The path for said file is derived from this property of the Trainer, which in turn derives from the save_dir of the Trainer's logger whenever the logger isn't a stock TensorBoard logger. The save_dir property of a LoggerCollection, which is what you get when the trainer is provided with more than one logger, is set to None by default, apparently deliberately. I'm not sure what the rationale behind that decision was, so I'm hesitant to make a PR myself, but it seems logical to me that LoggerCollection should have a log_path property defined analogously to that of a TensorBoardLogger, and that the property of the Trainer shouldn't fall back to the save_dir property automatically when its logger isn't an instance of TensorBoardLogger.

Reproduction

Here's a BoringModel Colab link. The following warning is generated during training when the Trainer's logger is wrapped in a list:

/usr/local/lib/python3.7/dist-packages/pytorch_lightning/profiler/pytorch.py:441: UserWarning: The PyTorchProfiler failed to export trace as `dirpath` is None
  rank_zero_warn("The PyTorchProfiler failed to export trace as `dirpath` is None")

Expected behavior

The PyTorch profiler should always have access to a default logging path, even when the Trainer's logger is a LoggerCollection. Ideally, it would follow the same "version" convention obeyed when the logger is a TensorBoardLogger.

Environment

* CUDA:
	- GPU:
		- Tesla T4
	- available:         True
	- version:           10.2
* Packages:
	- numpy:             1.19.5
	- pyTorch_debug:     False
	- pyTorch_version:   1.9.0+cu102
	- pytorch-lightning: 1.3.7post0
	- tqdm:              4.41.1
* System:
	- OS:                Linux
	- architecture:
		- 64bit
	- processor:         x86_64
	- python:            3.7.10
	- version:           #1 SMP Sat Jun 5 09:50:34 PDT 2021
@gahdritz gahdritz added bug Something isn't working help wanted Open to be worked on labels Jun 27, 2021
@tchaton tchaton added priority: 0 High priority task good first issue Good for newcomers labels Jun 28, 2021
@tchaton
Copy link
Contributor

tchaton commented Jun 28, 2021

Dear @gahdritz,

Thanks for reporting this issue. Would you be interested in making a fix PR to Lightning ?
I believe it shouldn't be too hard and a good way to learn more :)

Best,
T.C

@gahdritz
Copy link
Contributor Author

Yeah I can take a crack at it

@gahdritz
Copy link
Contributor Author

gahdritz commented Jul 1, 2021

@tchaton the PR is done. Any feedback is welcome.

@edenlightning edenlightning added this to the v1.3.x milestone Jul 1, 2021
@Borda Borda modified the milestones: v1.3.x, v1.4 Jul 6, 2021
@edenlightning edenlightning modified the milestones: v1.4, v1.3.x Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Open to be worked on priority: 0 High priority task
Projects
None yet
5 participants