Skip to content

Conversation

@bentsherman
Copy link
Member

Partial solution to #4725 (full solution also requires #3933)

Test case:

process AGGREGATE {
  container "quay.io/nextflow/bash"
  publishDir "results", mode: "copy"

  input:
  path(samples), stageAs: 'AnalysisFiles/'

  output:
  path("AnalysisFiles/*.txt", includeInputs: true)
  path("AnalysisFiles/Analysis_on_*")

  script:
  """
  for name in AnalysisFiles/*.txt; do
    touch AnalysisFiles/Analysis_on_\$(basename \${name} .txt)
  done
  """
}

workflow {
    AGGREGATE( files("files/*.txt") )
}

Where files should be populated with some text files. Run with Fusion enabled.

@netlify
Copy link

netlify bot commented Feb 8, 2024

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit b9429c0
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/66290ca084dfd200084cd8a4

@bentsherman
Copy link
Member Author

@marcodelapierre this one would be a good inaugural PR for the "merge to master" effort. It is a simple bugfix and does not affect the core runtime, only some auxiliary logic for Fusion symlinks. If you can review it and approve, I will merge it 👀

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Apr 16, 2024

I was attempting the test case described above. However I am getting an unrelated error, both with stable Nextflow and this build:

Unexpected path for Fusion script launcher: /Users/marco.delapierre/test/fusion-fix/work/45/681d22ba7f16b2567dc00f7a356900
$ ls files/
1.txt 2.txt
$ cat nextflow.config 
docker {
    enabled = true
}

fusion {
    enabled = true
}

wave {
    enabled = true
}

What am I missing?

(If I leave Fusion disabled, the example works)

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Apr 17, 2024

Just confirming I have the above issue when testing Fusion, with any of stable, edge, build from this PR. Both on x86 and ARM, also tried with a different container. Fusion version 2.2.13 (latest).

I must be missing something trivial in the config, but cannot spot it.

@bentsherman
Copy link
Member Author

I just uploaded the test case to github so that you can run it out-of-the-box:

nextflow run bentsherman/nf-tests -r fusion-symlinks

I couldn't reproduce your issue, but maybe you can try this example. Maybe you weren't mounting the AWS credentials into the container

Even stranger though, I just ran the test case with 23.10.1 and didn't reproduce the original issue of the symlinks not being resolved. When I list the work directory in S3, there is no .fusion.symlinks file and the input files are not symlinked.

@jordeu have you changed the behavior of Fusion symlinks? the above test case used to create Fusion symlinks but now it doesn't

@marcodelapierre
Copy link
Contributor

Thanks for the extra details Ben, by defining workDir and amazon credentials properly I could run the tests.

I confirm your finding with Nextflow 23.10.1 stable and any of the example workflows in #4725: all inputs and output files are published, and no .fusion.symlinks files are found in the S3 work directory.

I see this functional behaviour is also consistent with what a user reports in #4725.

I also tested the PR and confirm that the patch enables to correctly identify Fusion symlinks when running the workaround workflow in #4725 .

I am therefore approving the PR, as it effectively patches targeted case.

Of course, the key question remains of what change is now making the sample workflows to work.

Copy link
Contributor

@marcodelapierre marcodelapierre left a comment

Choose a reason for hiding this comment

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

Targeted corner case is fixed by the patch - effectiveness of resolveFusionLink .

I would still merge as this improves consistency of Fusion link resolution.

@bentsherman
Copy link
Member Author

False alarm, I set the publish mode to "copy" in the test which is another workaround

Fixing that, I now see the .fusion.symlinks in the folder and the symlink issue which is fixed by this PR. I will merge once the CI passes

@bentsherman
Copy link
Member Author

@pditommaso google batch logging test failed again

@pditommaso
Copy link
Member

This looks different

Method timed out after 600.00 seconds
	at [email protected]/java.lang.Thread.sleep(Thread.java:465)
	at nextflow.cloud.google.batch.logging.BatchLoggingTest.should fetch logs(BatchLoggingTest.groovy:135)

@bentsherman
Copy link
Member Author

Now the google test failed on 21 but not 11 🤔

@bentsherman
Copy link
Member Author

Consider closing in favor of #4967 (comment)

@bentsherman bentsherman closed this May 9, 2024
@bentsherman bentsherman deleted the fix-fusion-symlink-with-stageas branch May 9, 2024 16:20
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.

4 participants