Fix GpuUsageLogger to work on different platforms #3008

rohitgr7 · 2020-08-16T17:42:00Z

What does this PR do?

Fixes GpuUsageLogger to work on all platforms.
Also renamed it to GPUStatsMonitor: #3008 (comment)
Todo:

make the call to nvidia-smi once

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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.

Did you have fun?

Make sure you had fun coding 🙃

codecov · 2020-08-16T17:54:13Z

Codecov Report

Merging #3008 into master will decrease coverage by 9%.
The diff coverage is 85%.

@@           Coverage Diff            @@
##           master   #3008     +/-   ##
========================================
- Coverage      90%     81%     -9%     
========================================
  Files          84      84             
  Lines        8062    9240   +1178     
========================================
+ Hits         7244    7503    +259     
- Misses        818    1737    +919

pytorch_lightning/callbacks/gpu_usage_logger.py

mergify · 2020-08-17T20:22:28Z

This pull request is now in conflict... :(

awaelchli

I think the docs should say clearly that this is a callback and it uses the logger of the trainer.
Despite the name, we cannot do Trainer(logger=GpuUsageLogger()) which a user might think it is a real logger.

pytorch_lightning/callbacks/gpu_usage_logger.py

SkafteNicki

Maybe we should also add a test that checks that something is actually logged, when used in the proper way. Something like this should work (tested locally):

from pytorch_lightning.loggers import CSVLogger
from pytorch_lightning.loggers.csv_logs import ExperimentWriter
import os

@pytest.mark.skipif(not torch.cuda.is_available(), reason="test requires GPU machine")
def test_gpulogger(tmpdir):
    gpu_usage = GpuUsageLogger()
    logger = CSVLogger(tmpdir)
    model = EvalModelTemplate()
    trainer = Trainer(
        default_root_dir=tmpdir,
        callbacks=[gpu_usage],
        max_epochs=1,
        logger=logger
    )
    trainer.fit(model)
    path_csv = os.path.join(logger.log_dir, ExperimentWriter.NAME_METRICS_FILE)
    with open(path_csv, 'r') as fp:
        lines = fp.readlines()
    header = lines[0].split()

    fields = ['GPU_utilization.gpu',
              'GPU_memory.used',
              'GPU_memory.free',
              'GPU_utilization.memory'
              ]

    for f in fields:
        assert any([f in h for h in header])

pytorch_lightning/callbacks/gpu_usage_logger.py

rohitgr7 · 2020-08-18T19:39:42Z

Should it be renamed to GPUUsageLogger or GpuUsageLogger is fine?

SkafteNicki · 2020-08-18T20:31:47Z

Should it be renamed to GPUUsageLogger or GpuUsageLogger is fine?

I would keep it as it is, since the double big U makes it less readable in my opinion.

rohitgr7 · 2020-08-18T21:00:30Z

I would keep it as it is, since the double big U makes it less readable in my opinion.

or maybe GPUStatsLogger.

awaelchli · 2020-08-18T21:57:40Z

I like GPUStatsLogger more than GpuUsageLogger. Another question is whether it should be called a logger or not. I don't have a better suggestion though 👍

rohitgr7 · 2020-08-18T22:11:12Z

Yeah, maybe the name should be tweaked so that on should not confuse it with loggers.
I got few suggestions: MonitorGPUStats, GPUStatsMonitor, WatchGPUStats, GPUStatsWatcher

awaelchli · 2020-08-19T00:05:24Z

+1 for GPUStatsMonitor or just GPUStats

mergify · 2020-08-19T00:28:45Z

This pull request is now in conflict... :(

SkafteNicki · 2020-08-19T06:54:37Z

I guess it was named GpuUsageLogger because we also have a LearningRateLogger in callbacks. I also favor GPUStatsMonitor and we should then rename LearningRateLogger to LearningRateMonitor.

rohitgr7 · 2020-08-19T08:58:38Z

Yeah, will make changes in few hours and a will do a sep PR for learning rate logger name change.

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

rohitgr7 · 2020-08-27T17:28:24Z

I am for leaving it to another PR :]

Ok then, let's merge this and will continue changes on another PR :)
@Borda @awaelchli

awaelchli

@rohitgr7 need to ask @williamFalcon because he wants to do refactors which blocks all PRs, but this one is not touching any Trainer stuff so I think it should be fine.

rohitgr7 · 2020-08-27T17:46:52Z

yeah, It's just a callback, completely isolated from those things.

mergify bot requested a review from a team August 16, 2020 17:42

rohitgr7 commented Aug 16, 2020

View reviewed changes

pytorch_lightning/callbacks/gpu_usage_logger.py Outdated Show resolved Hide resolved

rohitgr7 force-pushed the fix_gpulogger branch from d9e0683 to d5b0496 Compare August 16, 2020 19:22

rohitgr7 changed the title ~~[WIP] Fix gpulogger~~ Fix gpulogger Aug 17, 2020

rohitgr7 changed the title ~~Fix gpulogger~~ Fix GpuUsageLogger to work on different platforms Aug 17, 2020

rohitgr7 force-pushed the fix_gpulogger branch from c57af7d to 6908dbf Compare August 17, 2020 20:23

Borda added the bug Something isn't working label Aug 17, 2020

rohitgr7 requested review from awaelchli and Borda August 18, 2020 11:48

awaelchli reviewed Aug 18, 2020

View reviewed changes

pytorch_lightning/callbacks/gpu_usage_logger.py Outdated Show resolved Hide resolved

pytorch_lightning/callbacks/gpu_usage_logger.py Outdated Show resolved Hide resolved

pytorch_lightning/callbacks/gpu_usage_logger.py Show resolved Hide resolved

mergify bot requested a review from a team August 18, 2020 12:13

SkafteNicki reviewed Aug 18, 2020

View reviewed changes

pytorch_lightning/callbacks/gpu_usage_logger.py Show resolved Hide resolved

mergify bot requested a review from a team August 18, 2020 13:09

rohitgr7 changed the title ~~Fix GpuUsageLogger to work on different platforms~~ [WIP] Fix GpuUsageLogger to work on different platforms Aug 18, 2020

rohitgr7 changed the title ~~[WIP] Fix GpuUsageLogger to work on different platforms~~ Fix GpuUsageLogger to work on different platforms Aug 18, 2020

rohitgr7 requested review from SkafteNicki and awaelchli August 18, 2020 20:27

SkafteNicki approved these changes Aug 18, 2020

View reviewed changes

mergify bot requested a review from a team August 18, 2020 20:34

awaelchli and others added 19 commits August 27, 2020 22:52

Apply suggestions from code review

80c43c7

update docs format

1086449

update docs

c429503

miss

bd433c8

merge

9d751d8

fix title formatting

de7a04a

unindent

916f0e4

punctuation

7c19232

simplify if statements

274145d

fix test

f4b0a3e

suggestions

22281fa

pep

e006e5b

Update CHANGELOG.md

07e18bd

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

fix on_train_batch_*

18a9b32

use AttributeDict

a4164da

usage

b2b3e1a

rank zero

9544ec4

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

import

7c1b8a8

minor changes

6da4857

rohitgr7 force-pushed the fix_gpulogger branch from b82c4a5 to 6da4857 Compare August 27, 2020 17:24

rohitgr7 changed the title ~~[WIP] Fix GpuUsageLogger to work on different platforms~~ Fix GpuUsageLogger to work on different platforms Aug 27, 2020

awaelchli approved these changes Aug 27, 2020

View reviewed changes

mergify bot requested a review from a team August 27, 2020 17:41

Borda approved these changes Aug 27, 2020

View reviewed changes

Borda merged commit f03943e into master Aug 27, 2020

Borda deleted the fix_gpulogger branch August 27, 2020 17:50

This was referenced Aug 28, 2020

Changed LearningRateLogger to LearningRateMonitor #3251

Merged

Refactor GPUStatsMonitor to improve training speed #3257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GpuUsageLogger to work on different platforms #3008

Fix GpuUsageLogger to work on different platforms #3008

rohitgr7 commented Aug 16, 2020 •

edited

Loading

codecov bot commented Aug 16, 2020 •

edited

Loading

mergify bot commented Aug 17, 2020

awaelchli left a comment

SkafteNicki left a comment

rohitgr7 commented Aug 18, 2020

SkafteNicki commented Aug 18, 2020

rohitgr7 commented Aug 18, 2020

awaelchli commented Aug 18, 2020

rohitgr7 commented Aug 18, 2020

awaelchli commented Aug 19, 2020

mergify bot commented Aug 19, 2020

SkafteNicki commented Aug 19, 2020

rohitgr7 commented Aug 19, 2020

rohitgr7 commented Aug 27, 2020

awaelchli left a comment

rohitgr7 commented Aug 27, 2020

Fix GpuUsageLogger to work on different platforms #3008

Fix GpuUsageLogger to work on different platforms #3008

Conversation

rohitgr7 commented Aug 16, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Aug 16, 2020 • edited Loading

Codecov Report

mergify bot commented Aug 17, 2020

awaelchli left a comment

Choose a reason for hiding this comment

SkafteNicki left a comment

Choose a reason for hiding this comment

rohitgr7 commented Aug 18, 2020

SkafteNicki commented Aug 18, 2020

rohitgr7 commented Aug 18, 2020

awaelchli commented Aug 18, 2020

rohitgr7 commented Aug 18, 2020

awaelchli commented Aug 19, 2020

mergify bot commented Aug 19, 2020

SkafteNicki commented Aug 19, 2020

rohitgr7 commented Aug 19, 2020

rohitgr7 commented Aug 27, 2020

awaelchli left a comment

Choose a reason for hiding this comment

rohitgr7 commented Aug 27, 2020

rohitgr7 commented Aug 16, 2020 •

edited

Loading

codecov bot commented Aug 16, 2020 •

edited

Loading