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

[Bug fix] : Fixed gsub issue in write.config.SIPNET #3275

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

meetagrawal09
Copy link
Collaborator

Description

Fixed issues with local deployment for SIPNET model.

The variables cpruncmd and cpoutcmd were added to write.config.SIPNET to extend it for cloud based PEcAn deployment. On a local deployment setup the workflow for SIPNET model was breaking with the below error :

Error in gsub("@CPRUNCMD@", cpruncmd, jobsh) :
  invalid 'replacement' argument
Calls: <Anonymous> ... <Anonymous> -> do.call -> write.config.SIPNET -> gsub
> proc.time()
   user  system elapsed
  7.132   3.362   7.877

The error arises since local pecan.xml does not contain settings$host$rabbitmq$cpfcmd and settings$host$rabbitmq$cpfcmd which is used to set cpruncmd and cpoutcmd in the write.config.SIPNET file.

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.
  • My name is in the list of CITATION.cff
  • 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.

jobsh <- gsub("@CPRUNCMD@", cpruncmd, jobsh)
jobsh <- gsub("@CPOUTCMD@", cpoutcmd, jobsh)
if(!is.identical(cpruncmd, character(0)) {
jobsh <- gsub("@CPRUNCMD@", cpruncmd, jobsh)
Copy link
Member

Choose a reason for hiding this comment

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

Couple questions:

  • are we sure that if these variables aren't defined in the pecan.xmxl that they'll be set to character(0)?
  • If these variables don't exist, are we sure the job.sh will run correctly with the text of these tags left in the shell script? looking at the template job, it seems like they should be removed (i.e. gsubbed with an empty string)

If the right thing to do is gsub with an empty string, and the is.identical is assuming the default is an empty string, this begs the question of why the default case is failing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code segment responsible to set the value of cpruncmd is write.configs.SIPNET.R#L76-L77.

Here since we don't set the value for settings$host$rabbitmq$cpfcmd as part of settings file that means on addressing this variable we get NULL. gsub returns a character(0) value when the target string is NULL.

Basically the line transforms to :

cpruncmd <- gsub("@OUTDIR@", settings$host$rundir, NULL)

which is why a check for this is needed in place.

Copy link
Member

Choose a reason for hiding this comment

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

@meetagrawal09 I agree with your diagnosis, but I think the issue should be fixed on lines 76-77 rather than here. Maybe that could be something like cpruncmd <- gsub("@OUTDIR@", settings$host$rundir, settings$host$rabbitmq$cpfcmd %||% ""), then the replacements on lines 105-106 remain unconditional?

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

3 participants