Skip to content
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

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

jedwards4b
Copy link
Contributor

@jedwards4b jedwards4b commented Jul 16, 2021

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

@jedwards4b jedwards4b self-assigned this Jul 16, 2021
Copy link
Member

@billsacks billsacks left a 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?

config/cesm/machines/config_compilers.xml Outdated Show resolved Hide resolved
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 '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spelling: tenser -> tensor

Copy link
Contributor Author

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 '
Copy link
Member

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

Copy link
Member

@billsacks billsacks left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants