CWL Model, Encoding & Decoding #421

caroott · 2024-07-19T08:54:18Z

This PR aims to add the F# CWL Data Model, as well as encoding and decoding functionality to interact with CWL files. It addresses issue #420

kMutagene · 2024-09-10T11:02:37Z

what's the status on this @caroott ?

caroott · 2024-09-10T11:42:41Z

still in progress :/
I hope to get it done soon™

kMutagene

Cannot say too much about the implementation, it looks good to me, but i really miss some xml docs on your types and functions. Ideally, also add a usage example to the docs folder

kMutagene · 2024-10-16T06:39:37Z

src/CWL/Decode.fs

+        Decode.object (fun get ->
+            Dirent
+                {
+                    // BUG: Entry Requires an Entryname to be present when it's an expression


Can you explain this, are you pushing code you know having a bug? 😆

I know that it is theoretically possible, but don't know how it would look/work then.

HLWeil

Thanks a lot for your efforts. I've got a few general requests (which I partly also put directly onto the files):

For Fable-compatability:
- Use [AttachMembers] attributes
- Use ResizeArrays as collections
Don't put top-level types into nested module structure. Using ARCtrl.CWL should be enoguh to make these types directly accessible. If necessary, adjust names to go with this change
For the test objects: Adjust names in a way to make clear that these were the CWL-FIle contents and not any datamodel oder intermediate representations. (I suggested to add "file", but maybe "fileContent" or something would be better.
Test file structure: Please adjust according to the other testfiles:
- Put a "main" object at the end with a TestList of the name of the class to be tested.
- Then rename the TestList containing the Tests to the behaviour of the class you want to test. E.g. in most of your cases you would have a testList "Decode"

HLWeil · 2024-10-16T10:30:28Z

src/ARCtrl/ARC.fs

@@ -55,7 +55,7 @@ module ARCAux =
            |> FileSystem.create
        fs.Union(tree)

-    let updateFSByCWL (cwl : CWL.CWL option) (fs : FileSystem) =       
+    let updateFSByCWL (cwl : CWL.CWLProcessingUnits.CWLToolDescription option) (fs : FileSystem) =       


Undo here, as it's the wrong type anways and might lead to confusion later. Use unit instead?

HLWeil · 2024-10-16T10:30:31Z

src/ARCtrl/ARC.fs

@@ -65,7 +65,7 @@ module ARCAux =


 [<AttachMembers>]
-type ARC(?isa : ArcInvestigation, ?cwl : CWL.CWL, ?fs : FileSystem.FileSystem) =
+type ARC(?isa : ArcInvestigation, ?cwl : CWL.CWLProcessingUnits.CWLToolDescription, ?fs : FileSystem.FileSystem) =


Undo here, as it's the wrong type anways and might lead to confusion later. Use unit instead?

HLWeil · 2024-10-16T11:22:07Z

src/CWL/ARCtrl.CWL.fsproj

+    <Compile Include="Encode.fs" />
+  </ItemGroup>
+  <ItemGroup>
+    <None Include="../../build/logo.png" Pack="true" PackagePath="\" />


You can drop this ItemGroup and Item

HLWeil · 2024-10-16T11:23:58Z

src/CWL/CWL.fs

+open Outputs
+open WorkflowSteps
+
+module CWLProcessingUnits =


Some of these types (the top-level ones) should definitely be directly in the ARCtrl namespace. Maybe take a look at ARCtrl.Core, where ArcAssay and such are directly in the ARCtrl namespace.

HLWeil · 2024-10-16T11:25:30Z

src/CWL/CWL.fs

+module CWLProcessingUnits =
+
+    type CWLToolDescription (
+            cwlVersion: string,


How vastly differing are the cwl versions? Do we have a default version in the specs? If not, we should pick one and set this field here to optional with a default value.

We ask for version 1.2 (current one) or higher. The version is a mandatory field in cwl files and always present, so it can always be read.

Yes, but this type is not exclusively for parsing but also for being created, edited and such. I don't think we should have this as a mandatory field in the constructor.

Even if you want to create or edit cwl, you need this field for it to be valid. So making it optional wouldn't make sense imo. A creator which automatically sets it to 1.2 could be an option.

Yes, having it as an optional parameter in the constructor with an internal default value is exactly what I meant.

HLWeil · 2024-10-16T11:42:09Z

tests/TestingUtils/TestObjects.CWL/CommandLineToolMetadata.fs

@@ -0,0 +1,108 @@
+module TestObjects.CWL.CommandLineToolMetadata
+
+let cwl ="""cwlVersion: v1.2


rename to "cwlFile"

HLWeil · 2024-10-16T11:42:29Z

tests/TestingUtils/TestObjects.CWL/CommandLineTool.fs

@@ -0,0 +1,39 @@
+module TestObjects.CWL.CommandLineTool
+
+let cwl ="""cwlVersion: v1.2


rename to "cwlFile"

HLWeil · 2024-10-16T11:44:09Z

tests/CWL/WorkflowSteps.Tests.fs

+    |> Decode.read
+    |> Decode.stepsDecoder
+
+let testWorkflowStep =


reorganize with "main" testList at the bottom

HLWeil · 2024-10-16T11:46:33Z

src/CWL/Decode.fs

+        let requirements = requirementsDecoder yamlCWL
+        let hints = hintsDecoder yamlCWL
+        let steps = stepsDecoder yamlCWL
+        printfn "%A" steps


debugging prints?

maybe 👀
It was telling me yesterday evening that it was empty, so i had to check^^

HLWeil · 2024-10-16T11:53:28Z

src/CWL/Decode.fs

+open WorkflowSteps
+open DynamicObj
+
+module Decode =


Remind to me to recheck this file when the structural change requests are implemented :D

HLWeil

After some consideration: Please omit all modules, i.e. put all type definitions in the top-level ARCtrl.CWL namespace.
Makes usage more straightforward and is consistent with the other types defined in ARCtrl namespace.
For this would be good to rename classes Input to CWLInput and Output to CWLOutput to minimize confusion.

HLWeil

Thanks for the adjustments. Looks good to me now 👍

caroott self-assigned this Jul 19, 2024

caroott force-pushed the CWL branch 2 times, most recently from a60e33d to 78e09de Compare October 14, 2024 10:40

caroott marked this pull request as ready for review October 15, 2024 18:13

caroott requested a review from HLWeil October 15, 2024 18:13

kMutagene requested changes Oct 16, 2024

View reviewed changes

HLWeil requested changes Oct 16, 2024

View reviewed changes

caroott added 21 commits October 21, 2024 08:08

Add cwl model

081ca04

Add cwl decode functions

ef04da1

Add testing project

8cfb04c

fix references in decode

63f8cdc

fix cwl type naming

cd3cdee

change naming of CWL to CWLProcessingUnits

3da6974

fix test fsproj

9c36d2d

add test strings

1932c44

Add cwl test

6ae8e47

fix optional fields

bbc5a42

rewrite tests to be more clear

a9e64d3

reorganize tests

a59d834

dix dockerFile requirement

3c5e2f8

fix EnvVarRequirement

86913f6

fix SchemaDefRequirement

27c8037

fix SoftwareRequirements

f4a662d

fix workdirReq and resourceReq

e44e736

start separating processunits for better modularity / edge case handling

3c6c0a3

reorder and specify wf specific types

13d8a9c

change in-/outputs to dynamicObj for more flexibility

c7a0082

add workflow steps

9930238

caroott added 16 commits October 21, 2024 08:14

support direct type mappings for inputs and outputs

95dab47

remove try with for dirent initial workdir

f53e73f

add overflow decoder

0fd5be9

add cmd tool metadata and tests

7007a0b

fix for new dynamic obj version

17fabe8

update fsproj

3fca0c1

Update YAMLicious to version 0.0.2

4ff39f9

rename Type to Type_ for fable

ff5af9c

add failcase to requirements

379efb4

add wf requirements to decoder

44112a8

add metadata and dynobj testing (fix test runs)

548b2b7

trim strings for python

8b6963e

update yamlicious version

7728ebb

add workflow decoding

1222f01

add cwl to all test list

83cad3e

rename types and tests

f452622

kMutagene force-pushed the CWL branch from 7c60965 to f452622 Compare October 21, 2024 06:26

caroott added 7 commits October 21, 2024 10:17

add attachmembers attribute

e6d9902

switch to resizearray and add some comments

070ca96

add optional field to inputs

090a214

separate processing units and remove module

10e40ba

change version and class to optional on tool and wf descriptions

de6ef63

remove type CLWClass :(

b783782

reorganize modules

fedc6d4

HLWeil requested changes Oct 21, 2024

View reviewed changes

remove underlying modules

844ca8b

HLWeil approved these changes Oct 21, 2024

View reviewed changes

HLWeil merged commit edb22da into main Oct 21, 2024
2 checks passed

kMutagene linked an issue Oct 23, 2024 that may be closed by this pull request

[Feature Request] Add ARC CWL Data Model #420

Closed

caroott deleted the CWL branch November 18, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CWL Model, Encoding & Decoding #421

CWL Model, Encoding & Decoding #421

caroott commented Jul 19, 2024

kMutagene commented Sep 10, 2024

caroott commented Sep 10, 2024

kMutagene left a comment •

edited

Loading

kMutagene Oct 16, 2024

caroott Oct 16, 2024

HLWeil left a comment

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

caroott Oct 16, 2024

HLWeil Oct 16, 2024

caroott Oct 16, 2024

HLWeil Oct 17, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

HLWeil Oct 16, 2024

caroott Oct 16, 2024 •

edited

Loading

HLWeil Oct 16, 2024

HLWeil left a comment

HLWeil left a comment

		@@ -0,0 +1,108 @@
		module TestObjects.CWL.CommandLineToolMetadata

		let cwl ="""cwlVersion: v1.2

		@@ -0,0 +1,39 @@
		module TestObjects.CWL.CommandLineTool

		let cwl ="""cwlVersion: v1.2

CWL Model, Encoding & Decoding #421

CWL Model, Encoding & Decoding #421

Conversation

caroott commented Jul 19, 2024

kMutagene commented Sep 10, 2024

caroott commented Sep 10, 2024

kMutagene left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HLWeil 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

Choose a reason for hiding this comment

caroott Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HLWeil left a comment

Choose a reason for hiding this comment

HLWeil left a comment

Choose a reason for hiding this comment

kMutagene left a comment •

edited

Loading

caroott Oct 16, 2024 •

edited

Loading