Add a check for external definitions that are not thread safe #920
Conversation
|
Logfiles from GitLab pipeline #72337 (:no_entry:) have been uploaded here! Status and direct links: |
alkino
force-pushed
the
check_thread_sade
branch
from
3699a27
to
b5f1eae
Compare
|
Logfiles from GitLab pipeline #72529 (:no_entry:) have been uploaded here! Status and direct links: |
|
And so we use some of them. What should we do? |
| const auto& name = node.get_node_name(); | ||
| if (details::is_external_definitions(name) && details::is_not_thread_safe(name)) { | ||
| logger->critical( | ||
| "SemanticAnalysisVisitor :: '{}' is not thread safe and incompatible with CoreNEURON", |
There was a problem hiding this comment.
I'm wondering whether instead of this message we should print that the problem is that these functions are not implemented in CoreNEURON (which will need to change if at some point we change the nmodl translator of NEURON with this NMODL).
@pramodk what do you think?
There was a problem hiding this comment.
I agree - the thread safety aspect is « implementation » aspect. I believe it’s not really about the method themselves.
Ioannis - as part of your work, we need to find out what needs to be done with these (legacy) functions.
And if you want to move this PR forward, throwing error as not implemented is better than saying not thread safe.
There was a problem hiding this comment.
@alkino @iomaganaris : something else relevant comes to my mind - if you look at the CI then it fails for netstim.mod for the usage of set_seed and exprand.
There are a few things to remember for thread safety: there are only certain blocks of the MOD file are called from the parallel region and they are: INITIAL, BREAKPOINT, DERIVATIVE, KINETIC, NET_RECEIVE (and something else?). If we are using non-thread-safe functions from these blocks (directly or indirectly) then only it's problem. Given this, I think we can bit improve this pass when to show a warning. For example:
-
set_seed(): this function is called fromPROCEDURE seed(x)but this procedure is not called fromINITIAL,BREAKPOINT,DERIVATIVE,KINETICorNET_RECEIVE. This means this procedure is most likely called from the interpreter (hoc/python) side in a serial execution. Hence this usage is not really problematic. -
exprand()is called indirectly from INITIAL/NET_RECEIVE->invl->erand so this usage is "potentially" problematic. But it's difficult to say for sure becauseexprandis preceded by the usage of VERBATIM block and hence this could be conditional usage for NEURON with#if !NRNBBCOREetc.
What I am trying to say is that it might be possible to do this check bit better? I haven't thought about the implementation aspect. If it's difficult, we can leave this. But I thought it's worth mentioning.
There was a problem hiding this comment.
@alkino : I am reviving this after 2 years!
As NEURON code generation is ongoing, I think this PR is relevant and good to have! If this PR is blocked for my earlier comment, especially set_seed and exprand, then I would say we can exclude these two functions and get the rest merged.
What do you think?
alkino
force-pushed
the
check_thread_sade
branch
from
b5f1eae
to
b76ade7
Compare
|
Logfiles from GitLab pipeline #73876 (:no_entry:) have been uploaded here! Status and direct links: |
|
I found this quite related: #249 |
Some functions are not thread safe and so not supported by CoreNeuron, handle them.