-
Notifications
You must be signed in to change notification settings - Fork 247
arch: prevent erroring at init in isolation #2815
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2815 +/- ##
==========================================
- Coverage 78.98% 78.97% -0.01%
==========================================
Files 248 248
Lines 50821 50822 +1
Branches 4391 4391
==========================================
Hits 40139 40139
- Misses 9879 9880 +1
Partials 803 803 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6344a03 to
512ff6f
Compare
| # cudart present but no driver detected. Likely isolation | ||
| # run such as version check or within a docker build. | ||
| return | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anti-pattern, fixing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to docs only driver can be 0 (NULL is invalid though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should 100% be warning users though, not blind returning from this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add the warning yes. It's highly unlikely a user works of see it though this merely only happens when you for example check devito.version within a docker build (which won't show the warning )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out my changes and see if you agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there were use cases where you might run Devito on a login node without the driver before deploying on a compute node with the driver (in which case warning but not failing seems like a good course of action)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a use case too yes. As of your change I personally prefer the library call and value check in separate statements but that's really not important so can just merge.
FYI was breaking the pro editable install because it was failing to check version during building
b6e852d to
a3937bb
Compare
a3937bb to
3bcf92f
Compare
3bcf92f to
8dad3b5
Compare
No description provided.