Skip to content

Add description of saladVersion #861

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add description of saladVersion #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5448ca8
Add description of `saladVersion`
tom-tan Aug 27, 2024
d4eef04
Add `saladVersion` to the metaschema
tom-tan Aug 29, 2024
15cb5bd
Update tests
tom-tan Aug 29, 2024
50107f0
Merge branch 'main' into saladVersion-description
tom-tan Aug 29, 2024
31bc518
Make `saladVersion` mandatory
tom-tan Sep 26, 2024
419c382
Merge branch 'main' into saladVersion-description
tom-tan Sep 26, 2024
55bf75e
Update metaschema-pre.yml
tom-tan Sep 26, 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
14 changes: 7 additions & 7 deletions schema_salad/metaschema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6929,7 +6929,7 @@ def save(
typedsl_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_or_array_of_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_2 = _TypeDSLLoader(
union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_or_array_of_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype,
2,
"v1.1",
"v1.3",
)
array_of_RecordFieldLoader = _ArrayLoader(RecordFieldLoader)
union_of_None_type_or_array_of_RecordFieldLoader = _UnionLoader(
Expand All @@ -6942,7 +6942,7 @@ def save(
union_of_None_type_or_array_of_RecordFieldLoader, "name", "type"
)
Record_nameLoader = _EnumLoader(("record",), "Record_name")
typedsl_Record_nameLoader_2 = _TypeDSLLoader(Record_nameLoader, 2, "v1.1")
typedsl_Record_nameLoader_2 = _TypeDSLLoader(Record_nameLoader, 2, "v1.3")
union_of_None_type_or_strtype = _UnionLoader(
(
None_type,
Expand All @@ -6956,7 +6956,7 @@ def save(
array_of_strtype, True, False, None, None
)
Enum_nameLoader = _EnumLoader(("enum",), "Enum_name")
typedsl_Enum_nameLoader_2 = _TypeDSLLoader(Enum_nameLoader, 2, "v1.1")
typedsl_Enum_nameLoader_2 = _TypeDSLLoader(Enum_nameLoader, 2, "v1.3")
uri_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_or_array_of_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_False_True_2_None = _URILoader(
union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype_or_array_of_union_of_PrimitiveTypeLoader_or_RecordSchemaLoader_or_EnumSchemaLoader_or_ArraySchemaLoader_or_MapSchemaLoader_or_UnionSchemaLoader_or_strtype,
False,
Expand All @@ -6965,11 +6965,11 @@ def save(
None,
)
Array_nameLoader = _EnumLoader(("array",), "Array_name")
typedsl_Array_nameLoader_2 = _TypeDSLLoader(Array_nameLoader, 2, "v1.1")
typedsl_Array_nameLoader_2 = _TypeDSLLoader(Array_nameLoader, 2, "v1.3")
Map_nameLoader = _EnumLoader(("map",), "Map_name")
typedsl_Map_nameLoader_2 = _TypeDSLLoader(Map_nameLoader, 2, "v1.1")
typedsl_Map_nameLoader_2 = _TypeDSLLoader(Map_nameLoader, 2, "v1.3")
Union_nameLoader = _EnumLoader(("union",), "Union_name")
typedsl_Union_nameLoader_2 = _TypeDSLLoader(Union_nameLoader, 2, "v1.1")
typedsl_Union_nameLoader_2 = _TypeDSLLoader(Union_nameLoader, 2, "v1.3")
union_of_None_type_or_booltype = _UnionLoader(
(
None_type,
Expand Down Expand Up @@ -7028,7 +7028,7 @@ def save(
union_of_None_type_or_array_of_SpecializeDefLoader, "specializeFrom", "specializeTo"
)
Documentation_nameLoader = _EnumLoader(("documentation",), "Documentation_name")
typedsl_Documentation_nameLoader_2 = _TypeDSLLoader(Documentation_nameLoader, 2, "v1.1")
typedsl_Documentation_nameLoader_2 = _TypeDSLLoader(Documentation_nameLoader, 2, "v1.3")
union_of_SaladRecordSchemaLoader_or_SaladEnumSchemaLoader_or_SaladMapSchemaLoader_or_SaladUnionSchemaLoader_or_DocumentationLoader = _UnionLoader(
(
SaladRecordSchemaLoader,
Expand Down
1 change: 1 addition & 0 deletions schema_salad/metaschema/metaschema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
saladVersion: v1.3
$base: "https://w3id.org/cwl/salad#"

$namespaces:
Expand Down
7 changes: 5 additions & 2 deletions schema_salad/metaschema/salad.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ enhance code generation by representing CWL data types as specific Python object
* Support for the Avro `map` schema
* Add named versions of the `map` and `union` Avro types
* Support for nested named `union` type definitions
* Add description of `saladVersion`

## References to Other Specifications

Expand Down Expand Up @@ -196,6 +197,7 @@ It is a fatal error if the document is not valid YAML.

A Salad document must consist only of either a single root object or an
Copy link
Contributor

@GlassOfWhiskey GlassOfWhiskey Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What happens to saladVersion in case of an array of objects? Each object must provide its own saladVersion? @mr-c @tetron

Copy link
Member Author

@tom-tan tom-tan Sep 12, 2024

Choose a reason for hiding this comment

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

At least, it is necessary to declare saladVersion at the root level.
Otherwise, we cannot decide whether to allow the newly introduced features such as nested typeDSL and map/union types.

I guess there are two options to declare saladVersion for the array of objects.

  1. As @GlassOfWhiskey mentioned, each object in the array must declare saladVersion
  2. I assume that the most root level documents use the explicit context. If my assumption is correct, it is reasonable to enforce saladVersion at the root level and to allow the imported documents inherit the saladVersion in the parent level.

Copy link
Contributor

@GlassOfWhiskey GlassOfWhiskey Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Option 1 is easier to be documented but it is less practical for users.
Option 2 requires to distinguish between root Salad documents and imported Salad documents (those that are included through the $import statement), clearly define the two concepts, and state that a Salad document can be used as a root Salad document only if it specifies a saladVersion field. Conversely, if imported documents do not specify a saladVersion they inherit the one from their root Salad doument.

Another problem here is dealing with mixed versions, as in CWL. What if a Salad 2.0 document imports a Salad 3.0 document, and viceversa? Is this something that Salad allows?

Copy link
Member Author

@tom-tan tom-tan Sep 17, 2024

Choose a reason for hiding this comment

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

Another problem here is dealing with mixed versions, as in CWL. What if a Salad 2.0 document imports a Salad 3.0 document, and viceversa? Is this something that Salad allows?

I prefer to take a similar approach as CWL.
That is,

  • Implementation allows a root document to import documents with newer and older saladVersion, and
  • Each imported document is validated and processed as its declared saladVersion rather than its parent saladVersion.

Here is the corresponding description in CWL v1.2.

Implementations should allow workflows to reference documents of both newer and older CWL versions (up to the highest version of CWL supported by that implementation). Where the runtime environment or runtime behavior has changed between versions, for that portion of the execution an implementation must provide runtime environment and behavior consistent with the document's declared version. An implementation must not expose a newer feature when executing a document that specifies an older version that does not include that feature.

Copy link
Member Author

@tom-tan tom-tan Sep 17, 2024

Choose a reason for hiding this comment

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

There is a technical issue for it.
The current code generator seems to hold saladVersion only at the root level rather than schema object level.

To fix this issue, we may need to introduce saladVersion for each schema object in addition to the root explicit context.

Copy link
Member

@mr-c mr-c Oct 4, 2024

Choose a reason for hiding this comment

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

@tom-tan Is this fixed?

Copy link
Member Author

@tom-tan tom-tan Oct 7, 2024

Choose a reason for hiding this comment

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

Not yet fixed because there is a corner case to be discussed as shown below:

I guess there are two options to declare saladVersion for the array of objects.

  1. As @GlassOfWhiskey mentioned, each object in the array must declare saladVersion
  2. I assume that the most root level documents use the explicit context. If my assumption is correct, it is reasonable to enforce saladVersion at the root level and to allow the imported documents inherit the saladVersion in the parent level.

@GlassOfWhiskey and I prefer the option 2 but it needs changes that may affect all the code generators.
There are several ways for option 2.

I prefer to introduce a upgrader for schema salad after fixing the typeDSL issue.
What do you think?

array of objects.
Each document must declare the `saladVersion` of that document. Implementations must validate against the document's declared version.
Copy link
Contributor

@GlassOfWhiskey GlassOfWhiskey Sep 8, 2024

Choose a reason for hiding this comment

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

This must here collides with the may clause at row 212.

Option 1: substitute this clause with

Each document may declare the saladVersion of that document. Implementations may validate against the document's declared version.

Option 2: substitute row 212 with

If a document consists of a root object, this object must contain the
field saladVersion and may contain the fields $base, $namespaces,
$schemas, and $graph:

Copy link
Member Author

@tom-tan tom-tan Sep 13, 2024

Choose a reason for hiding this comment

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

I prefer the option 2.
The option 1 makes difficult to solve #863 in the specification level.

Copy link
Contributor

@GlassOfWhiskey GlassOfWhiskey Sep 16, 2024

Choose a reason for hiding this comment

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

I totally agree to go with option 2 here.

Copy link
Member Author

@tom-tan tom-tan Sep 26, 2024

Choose a reason for hiding this comment

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

I fixed the row 212 to take the option 2.


## Document context

Expand All @@ -207,8 +209,9 @@ load the document. It may be overridden by an explicit context.

### Explicit context

If a document consists of a root object, this object may contain the
fields `$base`, `$namespaces`, `$schemas`, and `$graph`:
If a document consists of a root object, this object must contain the
field `saladVersion` and may contain the fields `$base`, `$namespaces`,
`$schemas`, and `$graph`:

* `$base`: Must be a string. Set the base URI for the document used to
resolve relative references.
Expand Down
245 changes: 122 additions & 123 deletions schema_salad/tests/metaschema-pre.yml
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion schema_salad/tests/test_makedoc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,5 +240,5 @@ def test_detect_changes_in_html(metaschema_doc: str, tmp_path: Path) -> None:
with open(result, "w") as h:
h.write(metaschema_doc)
assert (
hasher.hexdigest() == "3218d5a66d13025a874c0c05e0ad9af26d6131a4e27c019589b4aa18aee3c73d"
hasher.hexdigest() == "9df4c0c4cc13f814c66bc012202a700e197338beaa8f279b83b035d7e4a2cc04"
), result
Loading