[go: nahoru, domu]

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

Allow dynamic cache directives using closures #5044

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

Conversation

greenberga
Copy link
Contributor

Closes #5022

Copy link
netlify bot commented Jun 2, 2024

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit 23c5b64
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/665e1123560c2e000878ef53

@greenberga
Copy link
Contributor Author

@bentsherman I spent some time looking into this issue. There's a good chance I'm missing something, but as far as I can tell, the TaskConfig isn't aware of the hash mode. I can pull the cache property from task.config, but it's still a closure (i.e., it hasn't been evaluated).

Further, isn't the hash mode slightly different than whether or not to cache? The mode itself can be standard, lenient, deep, or sha256—but you can also set a boolean, in which case the mode will be null:

public static HashMode of( Object obj ) {
if( obj==null || obj instanceof Boolean )
return null;
if( obj instanceof CharSequence ) {
if( "true".equals(obj) || "false".equals(obj) )
return null;

Assuming there is some step where a script's process directives are merged with the directives from a config's process scope directives, it seems like you'd want to evaluate the closure at that point, in the context of the task. Let me know if that doesn't make sense. Thanks!

@greenberga greenberga force-pushed the greenberga/5022/dynamic-cache-directive branch from 21608b4 to 530f3b2 Compare June 2, 2024 14:33
cache = { task.tag != 'mytag' }
}
}
'''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently doesn't work because task is null when this closure gets evaluated. If you define a closure like this in a nextflow.config file, will it ever work? Isn't it attempting to capture a local variable that never exists in the context of the config file? In other words, tasks come from scripts, not config files. If closures work the way I understand them to, referencing the task variable in nextflow.config won't work because tasks don't get defined until the processor executes a script.

@bentsherman
Copy link
Member

I think you just need to move the getHashMode() method from the process config to the task config, something like this:

    HashMode getHashMode() {
        HashMode.of(get('cache')) ?: HashMode.DEFAULT()
    }

The get('cache') will handle the closure evaluation.

@greenberga
Copy link
Contributor Author
greenberga commented Jun 3, 2024

@bentsherman thanks for the point in the right direction. get('cache') does indeed seem to evaluate the closure properly. The only thing I can't quite figure out is why it's enough to move HashMode retrieval from the ProcessConfig to the TaskConfig. Whether get('cache') returns true or false, HashMode will be STANDARD.

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.

Request to enable dynamic cache directive
2 participants