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

Remote SDA launcher #2293

Merged
merged 156 commits into from
Jun 24, 2019
Merged

Remote SDA launcher #2293

merged 156 commits into from
Jun 24, 2019

Conversation

para2x
Copy link
Contributor

@para2x para2x commented Feb 15, 2019

After a series of checks, Using a function called SDA_remote_launcher I copy over all the required files for independently running SDA on remote. Along with the usual requirements for running SDA, I also copy a launcher file called SDA_launcher.R (located in inst/RemoteLauncher) which is responsible for calling the actual SDA function. This function is called through nohup and as a result it creates an output file capturing the progress of the SDA run on remote.
If launcher successfully start the nohup job it will return a folder path on remote as well as a PID corresponding to the SDA job. I will later use another function called Remote_Sync_launcher to launch a nohup syncing job on local monitoring the PID on the remote and syncing the outputs with local.

sda remote 1

Description

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@para2x para2x changed the title Remote SDA launcher {WIP} Remote SDA launcher Feb 18, 2019
@para2x para2x requested a review from serbinsh February 18, 2019 21:17
@para2x
Copy link
Contributor Author

para2x commented Feb 19, 2019

@mdietze this PR is ready for review. I will start writing the documentation as soon as you give me the green light on the implementation.

Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

In addition to the line-by-line changes, there doesn't seem to be any updates to the general documentation. This is too big of a change / feature addition to not have text explaining how it works and how it's used. In particular, new pecan xml tags definitely have to be documented.

modules/assim.sequential/R/Remote_helpers.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Remote_helpers.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Remote_helpers.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Remote_helpers.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Remote_helpers.R Show resolved Hide resolved
modules/assim.sequential/R/Remote_helpers.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/inst/RemoteLauncher/Remote_sync.R Outdated Show resolved Hide resolved
para2x and others added 11 commits June 19, 2019 09:21
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
…/02_hidden_analyses.Rmd

Co-Authored-By: Bailey(BNL) <34662202+bailsofhay@users.noreply.github.com>
…/02_hidden_analyses.Rmd

Co-Authored-By: Bailey(BNL) <34662202+bailsofhay@users.noreply.github.com>
Co-Authored-By: Bailey(BNL) <34662202+bailsofhay@users.noreply.github.com>
Co-Authored-By: Bailey(BNL) <34662202+bailsofhay@users.noreply.github.com>
Co-Authored-By: Shawn P. Serbin <sserbin@bnl.gov>
@para2x
Copy link
Contributor Author

para2x commented Jun 19, 2019

@serbinsh @bailsofhay I don't think I missed any comments. I addressed them all. If you think you don't have any more comments please approve this so I can ask Mike to do the final review.

@para2x
Copy link
Contributor Author

para2x commented Jun 20, 2019

@mdietze: I addressed @bailsofhay and @serbinsh comments and got their approval on this PR. I know it's a very large PR, but that'd be awesome if you could also give this a look when you have free time.

Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

In addition to my own requests, there were a lot of reasonable formatting improvements from Shawn that were marked as 'resolved' when they should have been merged in.

@@ -48,7 +48,7 @@ read_restart.SIPNET <- function(outdir, runid, stop.time, settings, var.names, p

# calculate fractions, store in params, will use in write_restart
wood_total_C <- ens$AbvGrndWood[last] + ens$fine_root_carbon_content[last] + ens$coarse_root_carbon_content[last]

if (wood_total_C<=0) wood_total_C <- 0.0001 # Making sure we are not making Nans in case there is no plant living there.
Copy link
Member

Choose a reason for hiding this comment

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

I know this was discussed in a thread somewhere, but remind me why this needs to be done (rather than relying on the GEF to treat this as a true zero)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we did that because in line 52-54, few variables are divided by wood_total_C and we didn't want to make NaNs.

modules/assim.sequential/R/Analysis_sda_multiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Analysis_sda_multiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/Analysis_sda_multiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
modules/assim.sequential/R/sda.enkf_MultiSite.R Outdated Show resolved Hide resolved
Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

Additional comment: this pull request seems to include the updated to the multi-site options for how Q is structured, but those concepts and settings flags are not included in the documentation. Also, the documentation for running SDA on remote (and enabling Restart) explains the comments, but not the flags that turn these feature on and off.

Copy link
Contributor

@araiho araiho left a comment

Choose a reason for hiding this comment

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

I'm really excited for this PR. It's awesome that you would be able to run the SDA runs remotely. I was wondering about the different dimensions of Q and if you have simulation scripts that prove that they are working correctly? I know that that has made a big difference for me in the past.

@mdietze mdietze merged commit bf337f2 into PecanProject:develop Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants