Skip to content

Conversation

@dreamgonfly
Copy link
Contributor

Before submitting

What does this PR do?

Fixes #452

Additionally, this PR updates & simplifies some code.

  1. Use subprocess.run as officially recommended by python docs (refer to https://docs.python.org/3/library/subprocess.html#using-the-subprocess-module)
  2. Clean up code around gpu memory function.
  3. Increase test coverage by adding test for get_memory_profile.

Did you have fun?

Yes :)

Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

looks fine, but some proper testing would be fine... (I understand that most free CI does not have GPU, so pls copy-paste results from your computer)

for k, v in zip(range(len(gpu_memory)), gpu_memory):
k = f'gpu_{k}'
gpu_memory_map[k] = v
gpu_memory = [int(x) for x in result.stdout.strip().split('\n')]
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of \n consider using os.sep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. I used os.linesep instead.

@dreamgonfly
Copy link
Contributor Author

This is metrics logs of training with min_max gpu memory logging option.
It shows two gpus with max & min utilization.

image

@williamFalcon
Copy link
Contributor

williamFalcon commented Nov 5, 2019

@dreamgonfly love the PR! thanks for the great submission.

@Borda i run every PR through GPU tests btw.

@williamFalcon williamFalcon merged commit 32dd803 into Lightning-AI:master Nov 5, 2019
@Borda
Copy link
Collaborator

Borda commented Nov 5, 2019

@Borda i run every PR through GPU tests btw.

I know, but for those who do not have avalaible couple of GPUs it is better to ask the author to show it... if you do not mind :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

min_max log_gpu_memory option bug

3 participants