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

Integrate with latest VODML tooling #20

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

Conversation

pahjbo
Copy link

@pahjbo pahjbo commented Aug 7, 2024

this builds on #19 to do some initial integration with https://ivoa.github.io/vo-dml/ tooling - it allows

gradle vodmlValidate

to be run - this does however show a lot of validation errors for the VO-DML - many initial ones related to ivoa/vo-dml#18

@pdowler
Copy link
Member

pdowler commented Aug 16, 2024

There is existing validation (up to and including VO-DML schematron) and there are warnings but no errors against the IVOA standard. I'm not sure what that means for the tooling from this PR.

Second major issue: we have a lot of code and repositories and use gradle 6.8.3 everywhere, so updating this to require gradle 8.x is not really viable at this time. Maybe when I get to working on the actual WD document and I want better doc generation tools it will become worth it.

@pahjbo
Copy link
Author

pahjbo commented Aug 16, 2024

There is existing validation (up to and including VO-DML schematron) and there are warnings but no errors against the IVOA standard. I'm not sure what that means for the tooling from this PR.

There are errors in the VO-DML that should have been picked up previously - e.g. uksrc@ce740cc - but that was the whole point of ivoa/vo-dml#18 - is trying to remove the ambiguities in the definitions that would allow mistakes like the above.

Second major issue: we have a lot of code and repositories and use gradle 6.8.3 everywhere, so updating this to require gradle 8.x is not really viable at this time. Maybe when I get to working on the actual WD document and I want better doc generation tools it will become worth it.

I understand that consistency is nice - however, the whole point of having the gradle wrapper present in the repo means that it is possible to have different gradle versions in each repo.

@pdowler
Copy link
Member

pdowler commented Aug 16, 2024

with respect to errors, are the published schema and schematron files not the definitive standard? If those say the VO-DML is valid.

Comparing the xsd and sch.xml files I'm using vs those on ivoa.net, I see that the schematron file is identical and the xsd is not, but I don't see any noted erratum saying when/what/why that changed. The diff is

NRC-068710:~>diff vo-dml-v1.xsd work/dev/opencadc/core.git/cadc-vodml/src/main/resources/vo-dml-v1.0.xsd
334a335,339
>         <xsd:attribute name="version" type="xsd:string" use="optional">
>             <xsd:annotation>
>                 <xsd:documentation>The major.minor version this VO-DML document.</xsd:documentation>
>             </xsd:annotation>
>         </xsd:attribute>

looks like a bug fix to add a missing attribute (ok).

So, I'm validating against the published standard and don't intend to do otherwise (too many moving parts). If the published standard is updated, then I'd obviously react to that. Given the issue you noted, I will take a look at that appendix and see if I've got some unintentional inconsistency (that's quite possible).

@pdowler
Copy link
Member

pdowler commented Aug 16, 2024

We added gradle wrapper so we could reliably run github CI, but it is not what developers or other build/test/release or development environments necessarily use (for various reasons).

I'd have to evaluate the impact of changing this repo (admittedly, now that I extracted it from the repo with the core java code, this repo doesn't actually build and release anything, so it is independent of most of that, but it isn't independent of developer environment and I don't like forcing an upgrade unless it's widely useful).

Consistency is often a wedge issue - once you drop a bit of it where it is OK, other things start going sideways and it is not OK :-)

for right now, I won't do anything. aside: I'm likely to be closing the smaller radio PR in favour of the more complete one (my caom25 branch)... and the plan is to review and merge that. So you may have to recreate your tooling PR again (from main) and that's a good way to keep track of it for when I get to the WD stage.

@pdowler
Copy link
Member

pdowler commented Aug 16, 2024

Oh, that xsd change removes the version attribute - presumably because it is redundant with the version element. I do not recall this coming up in the IVOA... was it a post-standard bug fix and I grabbed that xsd file too soon?

Our standard practice is to use a local copy of all xsd files (w3c doesn't allow java runtime to download xsd files because too many automated tests clobber their servers).

@pahjbo
Copy link
Author

pahjbo commented Aug 19, 2024

Oh, that xsd change removes the version attribute - presumably because it is redundant with the version element. I do not recall this coming up in the IVOA... was it a post-standard bug fix and I grabbed that xsd file too soon?

I was not involved in the original publishing of VO-DML, but presumably someone noticed that a version attribute was redundant/possibly contradictory, whether this was post publication or not I have no idea - what I did for consistency was copy back the published version into the git repository. It should be noted that the current tooling is working with what should be considered the WD1.1 version, and the current tooling is designed to carry all ancillary schema/schematron files with the gradle plugin.

However, the bulk of the validation errors that I mentioned are nothing to do with this but with ivoa/vo-dml#18 which concerns the form of the VO-DML ID - without repeating the whole of the email list argument, the crux of the matter is that he VODMLID element is defined not as an XML-ID but only as a restricted xs:string, so that there is no built-in uniqueness requirement - Appendix C described a recommended way of creating the vodml-ids that does make them unique and in fact the tooling does use this method - in particular when using the XMI/UML to VO-DML conversions. I suspect that CAOM might be the only model in the wild where the effect of making appendix C normative is noticeable as that XMI/UML->VO-DML tooling was not used in its creation.

@pdowler
Copy link
Member

pdowler commented Aug 19, 2024

So is the vodml id issue that some are not unique or that they are not built according to the suggested syntax in that appendix (that you feel could/should be normative)? If non-unique, it is clearly a bug... otherwise, it might be a diff between intention and reality and I'd have to think about the impact of such a change.

@pahjbo
Copy link
Author

pahjbo commented Aug 20, 2024

my preferred solution was to drop the vodml-Id elements entirely - the making the appendix C normative was the compromise

As the intention is that VO-DML models can be imported into other models for reuse then the importing model needs to know what the vodml-Id of an element is to refer to it - if the only requirement on a vodml-id is that it is unique within a document then the author of the imported model is legally able to publish a new version of the imported model definition changing all the vodml-ids thus breaking the importing model.

@pahjbo
Copy link
Author

pahjbo commented Sep 4, 2024

the fact that the working draft has section 4.8 and 4.9 labelled DataProductType illustrates the sort of problems that can occur too without DRYness

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.

2 participants