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.

Sign up for GitHub

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

update toil and resurrect tests #300

Merged
merged 14 commits into from
Dec 4, 2024
Merged

update toil and resurrect tests #300

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d65ad2c
Update config.yml
denis-yuen Nov 26, 2024
60f0666
Update install-tests.sh
denis-yuen Nov 26, 2024
d8cb679
looks like this was already broken in #8
denis-yuen Nov 27, 2024
cda6e2c
match existing packages
denis-yuen Nov 27, 2024
5122e51
some weird junit weirdness?
denis-yuen Nov 27, 2024
5deabba
new versions of toil seem to have a jobstore mandatory
denis-yuen Nov 27, 2024
44314f9
Revert "new versions of toil seem to have a jobstore mandatory"
denis-yuen Nov 27, 2024
3c8ad13
Reapply "new versions of toil seem to have a jobstore mandatory"
denis-yuen Nov 27, 2024
b101ce1
are bits of toil missing?
denis-yuen Nov 27, 2024
dc80b35
sigh @ python
denis-yuen Nov 27, 2024
0dfbcf1
bypass job store
denis-yuen Nov 27, 2024
479d125
works locally
denis-yuen Nov 27, 2024
68aff93
toil version string changed a bit
denis-yuen Nov 27, 2024
f044c70
pr feedback
denis-yuen Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ common_jobs: &common_jobs
- integration-tests:
matrix:
parameters:
testing_profile: [ "singularity-tests", "bitbucket-tests" ]
testing_profile: [ "singularity-tests", "bitbucket-tests", "toil-integration-tests" ]
<<: *common_filters
<<: *slack_context
requires:
Expand Down
10 changes: 10 additions & 0 deletions dockstore-client/src/main/java/io/dockstore/common/ToilOnlyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package io.dockstore.common;

public interface ToilOnlyTest {

String NAME = "io.dockstore.common.ToilOnlyTest";

default String getName() {
return NAME;

Check warning on line 8 in dockstore-client/src/main/java/io/dockstore/common/ToilOnlyTest.java

View check run for this annotation

Codecov / codecov/patch

dockstore-client/src/main/java/io/dockstore/common/ToilOnlyTest.java#L8 

Added line #L8 was not covered by tests
}
}
16 changes: 12 additions & 4 deletions dockstore-client/src/main/java/io/github/collaboratory/cwl/cwlrunner/ToilWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import io.dockstore.client.cli.ArgumentUtility;
import io.dockstore.client.cli.Client;
import io.dockstore.openapi.client.api.MetadataApi;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand All @@ -35,9 +37,9 @@ public void checkForCWLDependencies(MetadataApi metadataApi) {
final ImmutablePair<String, String> pair1 = io.cwl.avro.Utilities
.executeCommand(Joiner.on(" ").join(Arrays.asList(s1)), false, com.google.common.base.Optional.absent(),
com.google.common.base.Optional.absent());
final String toilVersion = pair1.getValue().trim();
final String toilVersion = pair1.getKey().trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to go look up that Utilities.executeCommand() returns a pair with stdOut and stdErr. For code readability, suggest a comment and/or a record.

Also, Toil changed where the version is displayed to stdOut from stdErr?

Copy link
Member Author

@denis-yuen denis-yuen Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah on the second point.
I'll add a ticket since executeCommand is overloaded and used in a bunch of places.
https://ucsc-cgl.atlassian.net/browse/SEAB-6828


final String expectedToilVersion = "3.15.0";
final String expectedToilVersion = "7.0.0";
if (!toilVersion.equals(expectedToilVersion)) {
ArgumentUtility.errorMessage("toil version is " + toilVersion + " , Dockstore is tested with " + expectedToilVersion
+ "\nOverride and run with `" + SCRIPT_FLAG + "`", Client.COMMAND_ERROR);
Expand All @@ -49,9 +51,15 @@ public List<String> getExecutionCommand(String outputDir, String tmpDir, String
//TODO: this doesn't quite work yet, seeing "toil.batchSystems.abstractBatchSystem.InsufficientSystemResources: Requesting more disk
// than either physically available, or enforced by --maxDisk. Requested: 537944653824, Available: 134853001216" on trivial
// workflows like md5sum

//try normalizing paths
Path currentRelativePath = Paths.get("");
Path tmpPath = Paths.get(currentRelativePath.toAbsolutePath().toString(), tmpDir);
denis-yuen marked this conversation as resolved.
Show resolved Hide resolved
tmpPath = tmpPath.normalize();

ArrayList<String> command = new ArrayList<>(
Arrays.asList("toil-cwl-runner", "--logError", "--outdir", outputDir, "--tmpdir-prefix", tmpDir, "--tmp-outdir-prefix",
workingDir, cwlFile));
Arrays.asList("toil-cwl-runner", "--bypass-file-store", "--logError", "--outdir", outputDir, "--tmpdir-prefix", tmpPath.toString(), "--tmp-outdir-prefix",
workingDir, cwlFile));
jsonSettings.ifPresent(command::add);
return command;
}
Expand Down
8 changes: 5 additions & 3 deletions dockstore-client/src/test/java/io/github/collaboratory/cwl/ToilLauncherIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@
*/
package io.github.collaboratory.cwl;

import io.dockstore.common.ToilOnlyTest;
import org.apache.commons.io.FileUtils;
import org.junit.Ignore;
import org.junit.experimental.categories.Category;
import org.junit.jupiter.api.Tag;

/**
* @author dyuen
*/
@Ignore
// @Category(ToilOnlyTest.class)
@Tag(ToilOnlyTest.NAME)
Copy link
Member Author

Choose a reason for hiding this comment

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

Something weird is going on here with junit and how this is being picked up by surefire/failsafe, may want to work on this together with updating them in general since we're out-of-date in this repo
https://ucsc-cgl.atlassian.net/browse/SEAB-6823

@Category(ToilOnlyTest.class)
public class ToilLauncherIT extends LauncherIT {

public String getConfigFile() {
Expand Down
2 changes: 1 addition & 1 deletion scripts/install-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ if [ "${TESTING_PROFILE}" = "unit-tests" ] || [ "${TESTING_PROFILE}" == "automat
fi

if [ "${TESTING_PROFILE}" = "toil-integration-tests" ]; then
pip3 install --user toil[cwl]==3.15.0
pip3 install --user toil[cwl]==7.0.0
else
pip3 install --user -r https://raw.githubusercontent.com/dockstore/dockstore/develop/dockstore-webservice/src/main/resources/requirements/1.15.0/requirements3.txt
fi
Expand Down