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.

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

Add description of saladVersion #861

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

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Aug 27, 2024

This request clarifies the existence of saladVersion, that is currently undocumented.

I'm afraid that added description may conflict with the existing description:

  • Added:

    Each document must declare the saladVersion of that document

  • Existing description that may conflict with added one:

    A Salad document must consist only of either a single root object or an array of objects.

Please let me know if there are better descriptions for it.

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.05%. Comparing base (19ac14b) to head (50107f0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #861   +/-   ##
=======================================
  Coverage   83.05%   83.05%           
=======================================
  Files          22       22           
  Lines        4886     4886           
  Branches     1326     1326           
=======================================
  Hits         4058     4058           
  Misses        548      548           
  Partials      280      280           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -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

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

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

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

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

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

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

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?

@@ -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
array of objects.
Each document must declare the `saladVersion` of that document. Implementations must validate against the document's declared version.
Copy link
Contributor

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

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

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

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.

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