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

MAINT: Disable use_hugepages in case of ValueError #16708

Merged
merged 1 commit into from Jun 30, 2020

Conversation

anirudh2290
Copy link
Member

@anirudh2290 anirudh2290 commented Jun 29, 2020

In accordance with this comment: #16679 (comment) , adding try except and setting use_hugepages to 0 in case of ValueError, since the additional LooseVersion import increases the import time a bit. From my measurement, it is more than 1 ms increase in import time.

Closes #16679.

EDIT: distutils import is more than 1 ms increase in import time.

@charris charris added 00 - Bug 03 - Maintenance 09 - Backport-Candidate PRs tagged should be backported and removed 00 - Bug labels Jun 29, 2020
@charris
Copy link
Member

charris commented Jun 29, 2020

It's good to put Closes #<issue_number> in the commit message so that the corresponding issue is automatically closed when the PR is merged.

@anirudh2290
Copy link
Member Author

Thanks @charris !

kernel_version = tuple(int(v) for v in kernel_version)
if kernel_version < (4, 6):
use_hugepage = 0
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth adding a comment here to explain why the try/except is necessary (perhaps referencing the issue)? A little context might help a future reader understand why it was structured this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the feedback @rossbar. i have added a comment here.

@seberg
Copy link
Member

seberg commented Jun 30, 2020

Tried to time it, and its possible that importing LooseVersion adds so little overhead that it would not matter. But, I suppose the try/except is maybe better in any case, just to be on the safe side, and it is not like we should be missing a significant portion of platforms.
Thanks Anirudh.

@seberg seberg merged commit a41d513 into numpy:master Jun 30, 2020
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: non-numeric kernel_version on the OVH platform
4 participants