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

Remove MemoryMonitor.py and test #56

Conversation

bbean23
Copy link
Collaborator

@bbean23 bbean23 commented Mar 28, 2024

This has somehow fixed #54. I'm not entirely sure what the issue was. My guess is that this fixes it because this reduces the possible interaction surface area between the garbage collector and the multiprocessing queue.

@bbean23 bbean23 force-pushed the 54-testmemorymonitor-test_large_memory_usage-unit-test-failing branch from b15dce8 to ff10235 Compare March 28, 2024 03:37
@bbean23 bbean23 marked this pull request as ready for review March 28, 2024 03:47
@bbean23 bbean23 requested review from e10harvey and braden6521 March 28, 2024 03:55
@bbean23 bbean23 self-assigned this Mar 28, 2024
@bbean23 bbean23 added the bug Something isn't working label Mar 28, 2024
@bbean23
Copy link
Collaborator Author

bbean23 commented Mar 28, 2024

closes #54

Copy link
Collaborator

@e10harvey e10harvey left a comment

Choose a reason for hiding this comment

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

@bbean23, thank you for these changes. In general, everything looks great. I do not understand what's causing the bug and need to before attempting to review a fix.

Do you have time to determine the cause of the bug with me today?

Should the test be disabled until the bug is resolved so that others can proceed with their PRs?

@braden6521
Copy link
Collaborator

@e10harvey's reasoning makes sense. Understanding the problem well before fixing makes sense to me. I'm on board with skipping in the meantime.

@bbean23 bbean23 force-pushed the 54-testmemorymonitor-test_large_memory_usage-unit-test-failing branch from ff10235 to 119cadd Compare March 28, 2024 22:41
@bbean23
Copy link
Collaborator Author

bbean23 commented Mar 28, 2024

As determined during our call today, the MemoryMonitor class has been removed, to be replaced with a third party tool like "top" on an as-needed basis.

@e10harvey @braden6521 would one of you mind approving? Thanks!

Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

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

Feel free to Merge! Thanks for doing this!

@bbean23 bbean23 dismissed e10harvey’s stale review March 28, 2024 22:59

talked in person, resolved w/ "removal" instead of change

@bbean23 bbean23 merged commit 347f796 into sandialabs:develop Mar 28, 2024
4 checks passed
@e10harvey e10harvey changed the title 54 testmemorymonitor test large memory usage unit test failing Remove MemoryMonitor.py and test Apr 1, 2024
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