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

Config-defined params using interpolated strings are stored as GStrings instead of Strings as of #4840 #4944

Closed
jmuhlich opened this issue Apr 23, 2024 · 6 comments · Fixed by #5020

Comments

@jmuhlich
Copy link

jmuhlich commented Apr 23, 2024

Bug report

Expected behavior and actual behavior

Prior to #4840 all string-valued params were stored in the ParamsMap as java.lang.Strings, but since then all interpolated strings are groovy GStrings. I noticed this when passing a string-valued param to my helper function with a String-typed arg started failing on 24.03.0-edge. I could add an explicit toString() call in my code but this change in Nextflow behavior was probably not intentional.

I think the key change was this line in a deleted block of code, where toString was originally called on any GStrings that aren't secrets (at least this is how I understand that code):
v24.02.0-edge...v24.03.0-edge#diff-06c5c01a6a87bb8d43db5f4132f1c562b768da582ae3b969d7b8d29d92687373L846
(ConfigBuilder.groovy line 846 in 24.02.0-edge, since that link seems flaky)

Steps to reproduce the problem

// test.nf
workflow {
    println 'foo class: ' + params.foo.getClass()
}
// test.config
params {
    foo = "$projectDir/bar"
}

command to run:

nextflow run test.nf -c test.config

Program output

expected (seen with 24.02.0-edge and earlier) :

foo class: class java.lang.String

actual (seen with #4840 / 00c9f22 and later including 24.03.0-edge) :

foo class: class org.codehaus.groovy.runtime.GStringImpl

Environment

  • Nextflow version: 24.03.0-edge
  • Java version: openjdk version "17.0.10-internal" 2024-01-16
  • Operating system: Linux - Ubuntu 22.04.4
  • Bash version: GNU bash, version 5.1.16(1)-release
@HarryHung
Copy link
Contributor

HarryHung commented May 22, 2024

Bumping this, facing the same issue here when I am testing Nextflow 24.04.1

The change of class from java.lang.String to org.codehaus.groovy.runtime.GStringImpl of String-type params causing a custom built helper function complaining about argument type mismatch.

As the author above, I would like to know whether it is intentional or unintentional before updating my codes. (I hope it is unintentional and will be reverted)

@pditommaso
Copy link
Member

Thanks for reporting this issue. Likely the change was made unintentionally; however the correct behavior should be the GString is coerced to String when invoking the function.

For example:

def x = 1
def y = "this $x"


String foo(String value) {
  return value
}


assert  y.getClass() instanceof GString
assert foo(y) == 'this 1'

Can you produce a test case showing the failure of your function?

@HarryHung
Copy link
Contributor

Further testing seems to demostrate that it requires several criteria altogether to trigger the unexpected behaviour:

  • The parameter is set through a config file
  • The parameter needs to include a Configuration Implicit Variable
  • The function taking the parameter is imported

This is the minimal reproducible example that should be able to reproduce the error

Directory Tree

.
├── main.nf
├── nextflow
├── nextflow.config
└── test_module.nf

nextflow is the executable at 24.04.1

main.nf

include { test_func } from "./test_module"

test_func(params.test_param)

test_module.nf

void test_func(String test_param) {
    log.info("Test Passed.")
}

nextflow.config

params.test_param = "$projectDir"

Running either ./nextflow run main.nf or ./nextflow run main.nf -c nextflow.config will lead to the error:

N E X T F L O W   ~  version 24.04.1

Launching `main.nf` [disturbed_pike] DSL2 - revision: 92559839ed

ERROR ~ argument type mismatch

 -- Check script 'main.nf' at line: 3 or see '.nextflow.log' file for more details

@pditommaso
Copy link
Member

pditommaso commented May 22, 2024

Ok, this will be solved by this PR #5020

@HarryHung
Copy link
Contributor

Thanks for the quick turnaround, so it will be part of the 24.04.2 update if I understand its label correctly?

@pditommaso
Copy link
Member

yes

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 a pull request may close this issue.

4 participants