womtool/miniwdl validate command #265

bshifaw · 2023-07-20T13:22:17Z

Added a command that validates wdl workflows using miniwdl package
Updated HelloWorld.wdl to verison1.0

…e option to use miniwdl. updated womtool function use in test scripts

bshifaw · 2023-07-28T18:37:43Z

tests/workflows/helloWorld.wdl

@@ -1,3 +1,5 @@
+version 1.0


Updated wdl to verison 1.0

bshifaw · 2023-07-28T18:40:23Z

tests/workflows/helloWorld.wdl

-
+        String docker
+    # Runtime Options:
+        Int mem


miniwdl failed this workflow because it didn't like operating on optional int with an int
mem * 1024
So made it none optional

lbergelson · 2023-08-02T19:07:42Z

I would consider running both validations by default. And doing both pre-submit.

…, moved miniwdl functions to miniwdl_utils.py

…e submitting workflow

…id workflows

bshifaw · 2023-08-16T19:11:01Z

src/cromshell/utilities/miniwdl_utils.py

+        f = io.StringIO()
+        with redirect_stdout(f):
+            # Importing miniwdl.check here to help with stdout redirection
+            from WDL.CLI import check as miniwdl_check


Not a fan of importing in the middle of the script but this seemed the only way the stdout redirection would work. If not imported here miniwdl's linting function will print out several warnings. Which isn't helpful when running the function in the submit command

Miniwdl just dumps all it's output to stdout then I'm assuming? This seems like a sane way to capture it.

bshifaw · 2023-08-16T19:13:06Z

src/cromshell/utilities/miniwdl_utils.py

+            )
+
+        captured_outputs = f.getvalue()
+    except Exception as exn:


Here i wanted to capture any exception from miniwdl, mainly to reprint it in a cleaner way.

That seems good to me. Definitely better to standardize them.

lbergelson

@bshifaw I have a few comments but otherwise this looks great. Thank you.

lbergelson · 2023-08-18T22:04:57Z

src/cromshell/__main__.py

@@ -168,15 +169,16 @@ def print_version():
 main_entry.add_command(alias.main)


Should we add a note to keep these alphabetical?

lbergelson · 2023-08-18T22:05:22Z

src/cromshell/submit/command.py

-            + ("\n".join(validate_status["errors"]))
-        )
-
+        LOGGER.debug("Skipping validation of WDL plus a dependencies zip")


There's no one working on fixing this particular cromwell issue is there? Should we poke them?

lbergelson · 2023-08-18T22:07:19Z

src/cromshell/utilities/miniwdl_utils.py

+            )
+
+        captured_outputs = f.getvalue()
+    except Exception as exn:


That seems good to me. Definitely better to standardize them.

lbergelson · 2023-08-18T22:11:15Z

src/cromshell/utilities/miniwdl_utils.py

+        captured_outputs = f.getvalue()
+    except Exception as exn:
+        # Importing miniwdl.print_error here to help with stdout redirection
+        from WDL.CLI import print_error as miniwdl_print_error


Is this still under the stdout redirect in this scope?

lbergelson · 2023-08-18T22:12:05Z

src/cromshell/utilities/miniwdl_utils.py

+        f = io.StringIO()
+        with redirect_stdout(f):
+            # Importing miniwdl.check here to help with stdout redirection
+            from WDL.CLI import check as miniwdl_check


Miniwdl just dumps all it's output to stdout then I'm assuming? This seems like a sane way to capture it.

lbergelson · 2023-08-23T15:19:22Z

src/cromshell/utilities/miniwdl_utils.py

+            # Importing miniwdl.check here to help with stdout redirection
+            from WDL.CLI import check as miniwdl_check
+
+            miniwdl_check(


I'm assuming miniwdl doesn't offer a clean way to get the results without just scraping the output logs? It would be cleaner if we didn't have to do that.

Not that I know of, having it directed to a variable didn't do anything

lbergelson · 2023-08-23T15:23:43Z

src/cromshell/validate/command.py

+@click.command(name="validate")
+@click.argument("wdl", type=click.Path(exists=True), required=True)
+@click.argument("wdl_json", type=click.Path(exists=True), required=False)
+@click.option(


In submit this is "--dependencies-zip. We should probably make this match. (Or maybe we should change it in submit?)

Miniwdl allows users to provide more than one folder to look for dependencies.
I can make it match the submitcommand so that users can only provide one folder (or zip) to send to miniwdl.

done, so now you can provide a zip or a path to a directory and it will provide it to miniwdl. I noticed that it doesn't handle nested workflows well (when the main workflow is in a child directory separate from its imported workflows). However it should be possible to reuse the same logic in the nested wdl PR to resolve it.

lbergelson · 2023-08-23T15:25:26Z

My comments are all basically questions, only real actionable thing is changing the argument name.

lbergelson

Looks good to me.

lbergelson · 2023-09-13T17:50:57Z

src/cromshell/validate/command.py

    type=click.Path(exists=True),
-    help="MiniWDL option: Directory containing workflow source files that are "
-    "used to resolve local imports. (can supply multiple times)",
+    help="MiniWDL option: ZIP file or directory containing workflow source files "


accessible to... what? WHAT is it accessible to? The mystery is killing me! :)

lbergelson · 2023-09-13T17:51:49Z

src/cromshell/validate/command.py

@@ -89,9 +89,11 @@ def main(
            wdl=str(wdl), wdl_json=str(wdl_json), config=config
        )

+    # Todo: Have it support nested directories after PR 268 is merged
+    # https://github.com/broadinstitute/cromshell/pull/268


lbergelson · 2023-09-13T17:52:38Z

tests/unit/conftest.py

+
+# Define a temporary directory for testing
+@pytest.fixture(scope="function")
+def temp_dir():


pytest fixtures are neat.

src/cromshell/validate/command.py

typo

bshifaw added 4 commits July 20, 2023 09:20

Added miniwdl validate command

667afcd

lint fixes

92b0804

Fix womtool validation error for version1.0

ecefdb9

update json for helloWorld.wdl

ee53592

bshifaw self-assigned this Jul 20, 2023

bshifaw linked an issue Jul 20, 2023 that may be closed by this pull request

Request to add a command to validate WDLs through cromshell. #261

Closed

bshifaw requested a review from lbergelson July 20, 2023 18:30

bshifaw changed the title ~~miniwdl validate command~~ womtool/miniwdl validate command Jul 24, 2023

bshifaw added 3 commits July 28, 2023 14:31

moved womtool functions to womtool_utils.py

4a70eab

Added exit code to utility test function

9d9ec00

switched validate command to by default validate via womtool, and hav…

804a5e8

…e option to use miniwdl. updated womtool function use in test scripts

bshifaw commented Jul 28, 2023

View reviewed changes

tests/workflows/helloWorld.wdl

@@ -1,3 +1,5 @@

version 1.0

Copy link

Collaborator Author

bshifaw Jul 28, 2023

Choose a reason for hiding this comment

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

Updated wdl to verison 1.0

bshifaw commented Jul 28, 2023

View reviewed changes

bshifaw added 4 commits August 8, 2023 16:52

both validators enabled by default, miniwdl validation when submiting…

91ec709

…, moved miniwdl functions to miniwdl_utils.py

lint fixes

2776025

Add option to hide miniwdl lint warnings, which hidden when run befor…

29aa2b9

…e submitting workflow

Add option to disable validation during testing to help test none val…

c762dfc

…id workflows

bshifaw commented Aug 16, 2023

View reviewed changes

lbergelson requested changes Aug 23, 2023

View reviewed changes

bshifaw added 5 commits August 30, 2023 09:54

Added note about keeping "main_entry.add_command" in alphabetical order

ddb21af

Work to make validate dependencies option similar to submit command

b0ba9d7

Refactoring of functions

5c6eb6a

added unit test for new functions, some lint fixes

ca78a8d

fix for resolved_deps function

dbc285a

lbergelson approved these changes Sep 13, 2023

View reviewed changes

bshifaw commented Sep 13, 2023

View reviewed changes

src/cromshell/validate/command.py Outdated Show resolved Hide resolved

Update src/cromshell/validate/command.py

de88a30

typo

bshifaw merged commit b32c718 into main Sep 13, 2023
8 checks passed

bshifaw deleted the bs_add_validate branch September 13, 2023 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

womtool/miniwdl validate command #265

womtool/miniwdl validate command #265

bshifaw commented Jul 20, 2023 •

edited

Loading

bshifaw Jul 28, 2023

bshifaw Jul 28, 2023 •

edited

Loading

lbergelson commented Aug 2, 2023

bshifaw Aug 16, 2023 •

edited

Loading

lbergelson Aug 18, 2023

bshifaw Aug 16, 2023

lbergelson Aug 18, 2023

lbergelson left a comment

lbergelson Aug 18, 2023

bshifaw Sep 5, 2023

lbergelson Aug 18, 2023

lbergelson Aug 18, 2023

lbergelson Aug 18, 2023

bshifaw Aug 30, 2023

lbergelson Aug 18, 2023

lbergelson Aug 23, 2023

bshifaw Sep 5, 2023

lbergelson Aug 23, 2023

bshifaw Aug 30, 2023

bshifaw Sep 5, 2023

lbergelson commented Aug 23, 2023

lbergelson left a comment

lbergelson Sep 13, 2023

lbergelson Sep 13, 2023

lbergelson Sep 13, 2023

		@@ -168,15 +169,16 @@ def print_version():
		main_entry.add_command(alias.main)

womtool/miniwdl validate command #265

womtool/miniwdl validate command #265

Conversation

bshifaw commented Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

bshifaw Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

lbergelson commented Aug 2, 2023

bshifaw Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Aug 23, 2023

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshifaw commented Jul 20, 2023 •

edited

Loading

bshifaw Jul 28, 2023 •

edited

Loading

bshifaw Aug 16, 2023 •

edited

Loading