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 swap from python ci #1714

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

talregev
Copy link
Contributor

Remove swap from python ci
It not needed for the ci.
Also may fix the hang problem.

@varunagrawal
Copy link
Collaborator

How do you know the swap space is not needed? Is there any analysis or memory dump showing this?

When we added the swap space, it was because the compilation would take too much memory, causing the VM to be killed. We have empirical evidence of that, however there is no evidence in this PR to the contrary.

@talregev
Copy link
Contributor Author

How do you know the swap space is not needed? Is there any analysis or memory dump showing this?

When we added the swap space, it was because the compilation would take too much memory, causing the VM to be killed. We have empirical evidence of that, however there is no evidence in this PR to the contrary.

Because I compile on the ci without it, and the ci don't crash on gcc out of memory.

@varunagrawal
Copy link
Collaborator

Because I compile on the ci without it, and the ci don't crash on gcc out of memory.

This is not a very convincing argument. I can make the opposite claim, which means nothing without evidence.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

I am approving this, we can revert this if it causes any issues.

@varunagrawal varunagrawal merged commit 448132a into borglab:develop Jan 17, 2024
31 checks passed
@talregev
Copy link
Contributor Author

talregev commented Jan 17, 2024

Because I compile on the ci without it, and the ci don't crash on gcc out of memory.

This is not a very convincing argument. I can make the opposite claim, which means nothing without evidence.

This is valid argument, because I show on this PR how I remove the swap and it still running and compiling and running the tests.
If what you say it true, according to your argument it should crash on every time without swap.

What is really happen, when you did your measurements tests linux ci was with low memory for gtsam with tbb compilation.
From that time, the ci was upgraded, now you have stronger machine, and your hack for more memory with swap cause probably to the ci to hang and not needed anymore.

@talregev
Copy link
Contributor Author

I am approving this, we can revert this if it causes any issues.

Thank you for approving. I agree. If we have issues we can later revet / fix them.

@varunagrawal
Copy link
Collaborator

This is valid argument, because I show on this PR how I remove the swap and it still running and compiling and running the tests. If what you say it true, according to your argument it should crash on every time without swap.

Sorry but this is not a valid argument. You may have just gotten lucky and the CI passed for you this time without the root cause being addressed.

What is really happen, when you did your measurements tests linux ci was with low memory for gtsam with tbb compilation. From that time, the ci was upgraded, now you have stronger machine, and your hack for more memory with swap cause probably to the ci to hang and not needed anymore.

This is conjecture. Can you please provide evidence for this?

Going forward I won't be approving PRs that make hand-wavy claims without the necessary discussion and evidence to back it up.

@talregev talregev deleted the TalR/remove_swap branch January 17, 2024 19:04
varunagrawal added a commit that referenced this pull request Feb 26, 2024
This reverts commit 448132a, reversing
changes made to 2dfd15e.
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.

2 participants