Add Nextflow support #546
Add Nextflow support #546
Conversation
There was a problem hiding this comment.
Disclaimer for this review, I have no experience with nextflow.
groovy should be installed in flake.nix
This can be done by adding nextflow-deps = [ pkgs.groovy ]; at line 84 and adding nextflow-deps to all-other-dependencies at line 86
If you require additional packages, also add them there.
When running tests on my local machine I get a lot of compilation errors and internal errors. As you've mentioned some of those specifically in your pr, let's go through them:
Limitations
io-function-additional-source-files and io-function-nested-call-exercise imposible
Which test are sensible to run for which languages is determined by tests/language_markers.py
It might be required to add extra categories here and use these in the specific tests.
Something similar was done for the bash language. Could maybe even be useful to write a function all_languages_except(*languages) which could then be used too run some tests for all languages except nextflow.
Even better would be to specify Construct.NESTED_CALL and Construct.EXTERNAL_SOURCE_FILES in such a way that langues can define whether or not this is supported. This way the error handling should be better when trying such an exercises are tried in the nextflow language. But I don't see how this can be implemented for Construct.EXTERNAL_SOURCE_FILES. This would also require some further alterations to TESTed that I should discuss with the team first. So for now I think the above solution is better.
Global variables may be possible but aren't supported for now
Simply not adding Construct.GLOBAL_VARIABLES to supported_constructs should cover this.
You can't pass argument to Nextflow scripts like normal programs, currently we do a simple mapping to params.p0..N the script has to know in advance how many arguments it will receive, this could be fixed by adding another parameter that gets set to N
Giving N as the first argument seems sensible if the amount of arguments needs to be known.
Issues
test batch-compilation-no-fallback-runtime is currently failing for nextflow
Do you have any more info on this or a specific question? If the test is irrelevant for next-flow, see my response above. Otherwise I would suggest debugging the testcase.
workflows get executed in parallel which means that you can't have multiple testcases in 1 Nextflow execution without the results being scrambled. This could be worked around with a modification to TESTed that allows either executing a command for each context or generating a script that in turn could start a new Nextflow execution per context. I would gratefully welcome feedback and advice on this though.
I wouldn't modify TESTed for this, as this seems a nextflow specific issue. Starting a new nextflow execution per context sounds like a good solution, but i don't know how much overhead this would add.
You can also try to give each workflow its own indexed output_streams and then combine those streams in the correct order with separators after all workflows are done.
But again, I have no idea of how nextflow works, so i am not sure what is most feasible.
Thanks for this, I think this is indeed an improvement for the test. |
|
Thank you for the review
I already tried this, this installs groovy 3.0.11 while at least 4.0 is needed. I have no experience with nix and am a bit lost in trying to solve this. AS for additional deps, the linter requires some jars and I'm unsure on how to include those. Currently these are optionally included in the resource path https://github.com/dodona-edu/universal-judge/pull/546/files#diff-bce7010e6183672925c659d52e129d047c7e67f56a429e2e4e7a6a4e75896189R27-R43
That's the case, but the tests run unconditionally. I've opened a different PR that skips these test for languages that don't support the construct #545
There is no "first" argument, arguments are named, so it would just be a convention of using specific names. |
I am no expert either. @rien could you give @Bond-009 some pointers on how to do this? |
|
Regarding the groovy version: the Running As for the linter, you can use Barring errors, it should probably look like this: nextflow-linter = let
codenarc = pkgs.fetchurl {
url = "https://repo1.maven.org/maven2/org/codenarc/CodeNarc/3.5.0/CodeNarc-3.5.0-all.jar";
hash = ""; # This wil complain that hashes do not match, simply copy and paste the expected hash here
};
linter-rules = pkgs.fetchurl {
# similar as above
};
slf4j-api = pkgs.fetchurl {
# similar as above
};
slf4j-simple = pkgs.fetchurl {
# similar as above
};
in pkgs.writeScriptBin {
name = "nextflow-linter";
text = ''
${pkgs.jre_minimal}/bin/java \
-Dorg.slf4j.simpleLogger.defaultLogLevel=error \
-classpath "${linter-rules}/linter-rules-0.1.jar:${codenarc}/CodeNarc-3.5.0-all.jar:${slf4j-api}/slf4j-api-1.7.36.jar:${slf4j-simple}/slf4j-simple-1.7.36.jar" \
"org.codenarc.CodeNarc"
'';
};If this is added to the I can make the snippet above more elegant and updatable by constructing the classpath from a list of package name, version and hash, if desired. |
|
Thanks @rien, |
|
Should I split this up into multiple PRs, for example one that updates the |
|
No need for a separate pr. Let me know when you are ready with all changes and the tests are working and I'll do another review :) |
Bond-009
force-pushed
the
feat/nextflow
branch
from
06551e7 to
22df238
Compare
|
The |
jorg-vr
added
run tests
|
All tests seem to succeed on github Linting is still failing |
Weird, this isn't the case locally |
jorg-vr
added
run tests
Not sure why CI is still failing on this, running it locally I don't get any errors anymore |
jorg-vr
added
run tests
jorg-vr
added
run tests
Removes the trailing whitespace from the input files. This simplifies the test cases and makes them more consistent.
Bond-009
force-pushed
the
feat/nextflow
branch
from
9231e49 to
14ede47
Compare
|
@jorg-vr Thank you for taking a look at the CI issues. |
jorg-vr
added
run tests
jorg-vr
added
run tests
jorg-vr
added
run tests
jorg-vr
added
run tests
|
The CI / types failure seems unrelated to my changes |
These are probably cause by upgraded dependencies due to nix upgrade. |
|
At this point in time I believe trying to add Nextflow support to the TESTed judge may have been a flawed idea. Some issues were expected from the beginning, with TESTed being designed for general purpose programming languages and Nextflow a DSL for parallel and scalable computational pipelines . Some Nextflow limitations we've had to work around include:
|
|
I think this is a valid analysis. Anyway it was an interesting experiment in exploring the limitations of our general purpose judge. For example the parallelism problem is something we might also have to resolve for more complex exercises in other languages and might also come up in courses on for example rust. I'll continue working on the move from nix to devcontainers, as this will also help us in other future updates. So that should solve at least one of the blocking issues for this pr. But keep me posted on your progress with the Nextflow specific judge, depending on your experience there we can decide if we want to close this pr or put in some extra effort to get it ready for production. |
Nextflow is an embedded DSL in Groovy. Which is why groovyc is used in the compilation step.
Limitations:
params.p0..Nthe script has to know in advance how many arguments it will receive, this could be fixed by adding another parameter that gets set to NIssues:
Test echo-function-file-input was altered to make the expected return value the same as the actual content of the file (the file contained a trailing newline which is now removed).
To implement this test the same solution as bash was chosen, a simple
cat. Despite this the Nextflow implementation failed the test, having included the trailing newline in the output.Based on this I believe that the send_value function bash uses is flawed and doesn't output the exact value it's provided. And that the test was incorrect to request the content without the trailing newline. Therefore I removed the trimming of the output from all solutions for this test.
This PR is made as part of an internship at VIB @vibbits with @abotzki and @MaybeJustJames