Safe setting of CPU Info #146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
as the context information might not be available on all devices, e.g. in our GitHub CI VM the CPU frequency. Wrap setting of dict entry in a try, except blog that throws warn- ings when a value cannot be obtained.
nicholasjng
reviewed
Jul 18, 2024
There was a problem hiding this comment.
I'd be in favor of a more tailored solution for the problem rather than having a None fallback for everything - cpu_freq() only seems to fail on macOS, and then we could always set frequency/min/max all to 0.
For example, if reading platform values really fails, you've got much bigger problems than running nnbench. So I'd prefer a try-except just around cpu_freq() if possible.
We decided on a more tailored approach and will only wrap the effected parts of the CPU info, not all of them.
as the function is not available on all devices, e.g. apple ARM in our GitHub CI. In that case, set frequency avg, min, max to zero.
as we set the CPU frequency to zero when it is not available on the host machine. That is the case in the GitHub CI. We don't want this test to fail when there is the frequency 0 as then the CPUInfo has been correctly populated.
as this is what is thrown by PSUTIL if the information is not available in the current machine.
nicholasjng
approved these changes
Jul 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
as the context information might not be available on all devices, e.g. in our GitHub CI VM the CPU frequency.
Wrap setting of dict entry in a try, except blog that throws warnings when a value cannot be obtained.
This is to address the CI failure described in #145 and very much a suggestion that we can discuss.
In any case, I think a failure of getting context info should not break the test execution in case they take some time.
As a data scientist using nnbench, I would want our suite to run through so i can react to failures. However, I definitely want information about missing values such that i can implement fixes to these in case they are crucial.