-
Notifications
You must be signed in to change notification settings - Fork 15
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
Adding warning message for demo server #975
Adding warning message for demo server #975
Conversation
This is for small deployment (max cpus localhost < 3), and when precise protocol is chosen. This should be indeed triggered for small deployment like the Demo Server.
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #975 +/- ##
==========================================
+ Coverage 68.40% 68.43% +0.02%
==========================================
Files 111 111
Lines 6293 6292 -1
==========================================
+ Hits 4305 4306 +1
+ Misses 1988 1986 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM @mikibonacci. Thanks!
if ( | ||
on_localhost and protocol == "precise" and localhost_cpus < 4 | ||
): # This means that we are in a small deployment. | ||
alert_message += ( | ||
f"Warning: The selected protocol is {protocol}, which is computationally demanding to run on localhost. " | ||
f"Consider the following: <ul>" | ||
+ suggestions["change_configuration"] | ||
+ "</ul>" | ||
) |
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.
Hey @mikibonacci. I didn't notice it in the review, but what is localhost_cpus
? It doesn't exist. What should be the logic here? Also, one of the tests failed?
This fixes #973
I will wait to merge this, as soon as #971 will be merged, so I can put the new variable name for the max cpus available in localhost (no more
localhost_cpus
).