-
Notifications
You must be signed in to change notification settings - Fork 212
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
code to link SmartRedis tools #4045
base: master
Are you sure you want to change the base?
Conversation
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.
In the interest of time, I have not reviewed the new python modules. If there are specific things you'd like me to look at with those, I can do so.
In addition to the inline comments, my main question is whether there's a way to do this that doesn't require an ifdef, in the interest of moving CESM to being more runtime configurable and relying less on build-time configurations. I don't see the whole picture here (since it seems like the important part may be in use nuopc_shr_methods, only : sr_client
), but I wonder if one possibility would be to at least isolate the ifdef at some low level by having a stub interface that can be used to compile the code if you're not compiling against the real smartsim library? So client code like this could always rely on there being an sr_client
that defines put_tensor
and unpack_tensor
methods – but in some cases these would just be do-nothing stubs. Then this client code could use simple conditionals (if (use_smartsim)
) rather than an ifdef, and there could (ideally) just be a single ifdef at a low level. I'm not sure if this is possible, and how hard it would be, so I'll let you make the call as to whether it's feasible. Let me know if you'd like to discuss it further.
If you do stick with an ifdef, does this need to be added to the sharedlib path for the relevant libraries?
call random_number(true_array_real_64) | ||
call random_number(recv_array_real_64) | ||
write(key_prefix, "(A,I6.6)") "pe_",mytask | ||
if(mastertask) write(logunit, *) 'putting tenser to smartsim database ' |
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.
Nit: spelling: tenser -> tensor
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.
corrected, thanks
write(key_prefix, "(A,I6.6)") "pe_",mytask | ||
if(mastertask) write(logunit, *) 'putting tenser to smartsim database ' | ||
call sr_client%put_tensor(key_prefix//"true_array_real_64", true_array_real_64, shape(true_array_real_64)) | ||
if(mastertask) write(logunit, *) 'retreveing tenser from database ' |
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.
Nit: spelling: tenser -> tensor; retreveing -> retrieving
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.
Thanks a lot for the quick changes - looks good!
0ac417e
to
f7181f9
Compare
Add an xml variable USE_SMARTSIM which will cause cmeps models to be built with an interface to the
HPE SmartSim/SmartRedis database.
Test suite: hand testing, scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status: bit for bit
Fixes [CIME Github issue #]
User interface changes?:
Update gh-pages html (Y/N)?:
Requires PR's ESCOMP/CESM_share#5
and ESCOMP/CMEPS#213