Skip to content

DEVEX-2578 dxCompiler Option for Parallel Compilation of Applets [by Fulcrum Genomics]#510

Merged
r-i-v-a merged 22 commits intodevelopfrom
for_testing_fulcrum_genomics_pr
Jun 30, 2025
Merged

DEVEX-2578 dxCompiler Option for Parallel Compilation of Applets [by Fulcrum Genomics]#510
r-i-v-a merged 22 commits intodevelopfrom
for_testing_fulcrum_genomics_pr

Conversation

@r-i-v-a
Copy link
Copy Markdown
Member

@r-i-v-a r-i-v-a commented Jun 3, 2025

DEVEX-2579 For testing Fulcrum Genomics PR

@r-i-v-a r-i-v-a added the integration2 Integration tests with dxcint label Jun 3, 2025
@r-i-v-a r-i-v-a removed the integration2 Integration tests with dxcint label Jun 10, 2025
r-i-v-a and others added 2 commits June 10, 2025 14:20
---------

Co-authored-by: Ted Brookings <ted@fulcrumgenomics.com>
@r-i-v-a r-i-v-a added cwl_conformance Triggers CWL conformance tools and workflows tests and removed cwl_conformance Triggers CWL conformance tools and workflows tests labels Jun 10, 2025
---------

Co-authored-by: Ted Brookings <ted@fulcrumgenomics.com>
@r-i-v-a r-i-v-a requested a review from aogorodnikov-dna June 11, 2025 10:43
r-i-v-a and others added 2 commits June 16, 2025 17:14
* Fix typo in test invocation of -executableCreationParallelism

* Maybe fix applet with dockerRegistry test failing due to change in order of independent tasks

---------

Co-authored-by: Ted Brookings <ted@fulcrumgenomics.com>
…order is reversed (#519)

Co-authored-by: Ted Brookings <ted@fulcrumgenomics.com>
@r-i-v-a r-i-v-a requested review from YuxinShi0423 and mhrvol June 17, 2025 17:23
set -euxo pipefail

TEST=${1}
TEST=${1:-""}
Copy link
Copy Markdown
Member Author

@r-i-v-a r-i-v-a Jun 17, 2025

Choose a reason for hiding this comment

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

  • revert script

@r-i-v-a
Copy link
Copy Markdown
Member Author

r-i-v-a commented Jun 17, 2025

  • Add documentation about -executableCreationParallelism <int>

@r-i-v-a
Copy link
Copy Markdown
Member Author

r-i-v-a commented Jun 17, 2025

  • Add changelog note

@r-i-v-a r-i-v-a marked this pull request as ready for review June 19, 2025 21:54
@r-i-v-a r-i-v-a added integration2 Integration tests with dxcint cwl_conformance Triggers CWL conformance tools and workflows tests labels Jun 19, 2025
@r-i-v-a r-i-v-a changed the title DEVEX-2579 For testing Fulcrum Genomics PR DEVEX-2578 dxCompiler Option for Parallel Compilation of Applets [by Fulcrum Genomics] Jun 20, 2025
@r-i-v-a r-i-v-a removed cwl_conformance Triggers CWL conformance tools and workflows tests integration2 Integration tests with dxcint labels Jun 23, 2025
Comment on lines +981 to +988
if (desc1.access == expected2Access && desc2.access == expected1Access) {
fail("Compiled applet order is reversed")
}

desc1.access shouldBe expected1Access

desc2.access shouldBe expected2Access

Copy link
Copy Markdown
Contributor

@YuxinShi0423 YuxinShi0423 Jun 25, 2025

Choose a reason for hiding this comment

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

I dont get it why the access order could be wrong. The parallelized compilation is done on callable, and the network access is embedded in the callable object, so it would go with the callable and should not be affected by this new setting. @r-i-v-a

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@YuxinShi0423 I think the test is just asserting that the new setting doesn't change the order; this was mentioned in the spec attached to the PMUX ticket.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had introduced a bug that did reverse the order, but the resulting error message was unclear (it looked like the results were just wrong). I made the message for reversed order more clear and fixed the bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@TedBrookings could you point me to the commit for that fix?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

d56c1cb Merge branch 'for_testing_fulcrum_genomics_pr' of github.com:dnanexus/dxCompiler into for_testing_fulcrum_genomics_pr
The changes were to Compiler.scala and ExecutableTree.scala. That said, this was fixing a bug that I had introduced earlier. I had been using Map keys instead of Vectors to track dependencies, so the order of mutually-independent executables had not been preserved correctly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! So the access config is just the flag we used to check if applet1 and 2 are in correct order, but itself wont not be swapped.
I would still prefer another unit test since this one serves two purposes now. But I am okay with it. @r-i-v-a

@r-i-v-a r-i-v-a requested a review from YuxinShi0423 June 27, 2025 00:13
Copy link
Copy Markdown
Contributor

@YuxinShi0423 YuxinShi0423 left a comment

Choose a reason for hiding this comment

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

I got no more question. Thanks! LGTM

@r-i-v-a r-i-v-a merged commit 4881243 into develop Jun 30, 2025
18 of 20 checks passed
@r-i-v-a r-i-v-a deleted the for_testing_fulcrum_genomics_pr branch June 30, 2025 21:22
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.

3 participants