Try to improve version checks in VOTable by timj · Pull Request #18366 · astropy/astropy

timj · 2025-06-28T18:32:12Z

Description

This pull request is to address some issues I encountered creating a VOTable programmatically.
It doesn't solve all the problems in that I'm still not clear how I'm supposed to instantiate a TimeSys object since it's not clear how I am supposed to be able to get the version compatibility config from a VOTableFile, but it does at least solve part of the problem by allowing TableElement to know its own version from the associated VOTableFile.

Fixes #18356

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-06-28T18:32:47Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

timj · 2025-06-28T21:14:43Z

+        parameter unchanged.
+    """
+    if config is None:
+        if hasattr(votable, "_get_version_checks"):


Now that I've made it so that VOTableFile always has the version checks set for it, it's probably better to make this line instead be:

Suggested change

if hasattr(votable, "_get_version_checks"):

if hasattr(votable, "config"):

and then copy the entire config over.

That's a good point. I don't have a strong preference, though limiting the scope to the version configs makes it a little more explicit why this is being done and is closest to what was being done before with Field. Copying the whole config could bring in config items from the votable, which is probably not harmful and might even be good in most cases, but it does go beyond what is explicitly needed for this fix.

It's messy that version concerns aren't better encapsulated (away from configs). Without bigger refactoring, it will be hard to make configs really safe during programmatic construction of a votable, i.e., that they meet all the same implicit assumptions as when they are created during parsing. The API allows passing anything in for config in each element constructor.

Ok. I'll leave it with the explicit _get_version_checks call, although I will add "version" field into the dict so that all the version information is present.

timj · 2025-06-28T21:16:44Z

It occurred to me that it would be much easier if VOTableFile was changed to:

Always include the version checks in its internal config (and the version number)
Had a public config property

Since then when constructing a VOTable programmatically (and not from parsing an XML file) you can reliably pass in config=votable.config to every constructor that needed a config and things would Just Work. I made that change and added some tests which do now make it clear how to construct a TimeSys without a warning.

pllim · 2025-06-30T15:24:43Z

            if element is before:
                if getattr(element, attr, None) == ref:
                    vo_raise(
+                        KeyError,


How did this ordering error not trigger in CI for the past 14 years? 😱

My working theory is that the code has always raised (since the code assumes an exception class but falls over when it's given a string) but nothing in CI was in fact checking the actual exception class.

Even if not directly this may feed back to the issues that test coverage is not great or not sufficient (#9433 but some others, too); and that we should really switch to mandatory kwargs as much as possible (#17224)

pllim · 2025-06-30T15:26:04Z

Need change log. Thanks!

timj · 2025-06-30T15:37:05Z

Will add a changelog once I get some feedback that I'm on the right track.

tomdonaldson

Thanks for tackling this @timj. I think this is a very reasonable approach to addressing the missing config values. I appreciate the nice new test and the code cleanup changes you made along the way.

It looks like there are a couple minor CI items (a ruff complaint and needing a changelog over in docs/changes/io.votable). I'll be happy to approve when that's in place. Let us know if we can help with those items.

tomdonaldson · 2025-07-10T00:35:13Z

+        parameter unchanged.
+    """
+    if config is None:
+        if hasattr(votable, "_get_version_checks"):


That's a good point. I don't have a strong preference, though limiting the scope to the version configs makes it a little more explicit why this is being done and is closest to what was being done before with Field. Copying the whole config could bring in config items from the votable, which is probably not harmful and might even be good in most cases, but it does go beyond what is explicitly needed for this fix.

It's messy that version concerns aren't better encapsulated (away from configs). Without bigger refactoring, it will be hard to make configs really safe during programmatic construction of a votable, i.e., that they meet all the same implicit assumptions as when they are created during parsing. The API allows passing anything in for config in each element constructor.

This lets the version checks succeed.

It needs an exception class.

Cannot use the version setter because we allow v1.0 files to be read from XML even though we don't allow someone to create one with the APIs directly.

This ensures that error messages can report the actual version without having to work it out from the or_later values.

timj · 2025-07-11T20:54:51Z

@tomdonaldson looks like all checks pass now.

timj · 2025-07-11T21:32:00Z

@tomdonaldson whilst I was testing my upstream code, I noticed that Astropy VOTable output fails validation, at least for v1.5. Because the Resource.to_xml() method does:

https://github.com/astropy/astropy/blob/main/astropy/io/votable/tree.py#L4038-L4044

            element_sets = [
                self.coordinate_systems,
                self.time_systems,
                self.params,
                self.infos,
                self.links,
            ]

but the schema requires that INFO comes before COOSYS. This causes stilts votlint to complain:

ERROR (l.8, c.59): cvc-complex-type.2.4.a: Invalid content was found starting with element '{"http://www.ivoa.net/xml/VOTable/v1.3":INFO}'. One of '{"http://www.ivoa.net/xml/VOTable/v1.3":COOSYS, "http://www.ivoa.net/xml/VOTable/v1.3":TIMESYS, "http://www.ivoa.net/xml/VOTable/v1.3":GROUP, "http://www.ivoa.net/xml/VOTable/v1.3":PARAM, "http://www.ivoa.net/xml/VOTable/v1.3":LINK, "http://www.ivoa.net/xml/VOTable/v1.3":TABLE, "http://www.ivoa.net/xml/VOTable/v1.3":RESOURCE, WC[##other:"http://www.ivoa.net/xml/VOTable/v1.3"]}' is expected.

Trivial fix if you want me to add it on this ticket.

timj · 2025-07-11T21:42:53Z

Looking at the history I think Astropy VOTables with COOSYS and INFO in the RESOURCE block have been failing validation since the beginning of time (I tracked the problem back to at least 2011).

timj · 2025-07-11T21:46:17Z

Since it was a one line fix I fixed it here.

timj · 2025-07-11T21:57:20Z

Clearly some tests hardwire the wrong order... [fixed]

tomdonaldson

@timj Nice catch and thanks for the fix. I'm glad you've been pushing on the votable creation and writing aspects of this module, and that (at least so far) the issues have been manageable. Clearly those use cases could use a lot more test coverage.

The INFO elements have been a subject of some confusion for a while. It's always been weird that they have been allowed both at the beginning and end of VOTABLE and RESOURCE, but that in VOTABLE the order of the first one didn't matter. Another area of confusion was that up through v1.3, the usually helpful figures summarizing the elements got the ordering requirement wrong in RESOURCE (subsequently fixed in v1.4).

This all looks good now.

github-actions Bot added the io.votable label Jun 28, 2025

timj force-pushed the u/timj/votable-config branch from 8dafbcd to 236f9ff Compare June 28, 2025 21:11

timj commented Jun 28, 2025

View reviewed changes

timj force-pushed the u/timj/votable-config branch 3 times, most recently from 6c54d56 to bec861c Compare June 28, 2025 22:33

pllim reviewed Jun 30, 2025

View reviewed changes

pllim added this to the v7.2.0 milestone Jun 30, 2025

pllim added the Bug label Jun 30, 2025

pllim requested review from bsipocz and tomdonaldson June 30, 2025 15:37

tomdonaldson requested changes Jul 10, 2025

View reviewed changes

timj added 13 commits July 11, 2025 12:46

Use config.get to avoid KeyError for version checks

dd9e741

Prefill VOTable version config where possible

1a5fc10

This lets the version checks succeed.

Fix vo_raise call for bad format

ad37469

It needs an exception class.

Fix vo_raise calls so that the exception comes as first parameter

8e0cf74

Add test for BINARY2 being allowed

643c2e0

Say that BINARY2 is allowed now in warning message

1809a61

Add VOTableFile.config property and ensure it always has version params

3d6e9e9

Fix version look up in error message.

fb1c73d

Add tests for version differences in CooSys and TimeSys

44eac5f

Reduce duplication of version check logic in VOTableFile

0f8e148

Force update of version of VOTableFile during parse

e66fb62

Cannot use the version setter because we allow v1.0 files to be read from XML even though we don't allow someone to create one with the APIs directly.

Add a test for Param field to check warning path

4cb684b

Reformat with ruff

f7bf1ed

timj force-pushed the u/timj/votable-config branch from d7de74c to f7bf1ed Compare July 11, 2025 19:46

timj added 3 commits July 11, 2025 13:02

Ruff format

f18307f

Store the VOTable version in the version check config

32b8861

This ensures that error messages can report the actual version without having to work it out from the or_later values.

Add news fragments

0858cf4

timj mentioned this pull request Jul 11, 2025

DM-51564: Add COOSYS and TIMESYS to self-description template lsst-sqre/sia#57

Merged

timj force-pushed the u/timj/votable-config branch from ca8ae4d to ede16c4 Compare July 11, 2025 22:12

tomdonaldson approved these changes Jul 16, 2025

View reviewed changes

tomdonaldson merged commit dfcc608 into astropy:main Jul 16, 2025

stvoutsin mentioned this pull request Jul 24, 2025

Add benchmarks for VOTable binary parsing astropy/astropy-benchmarks#141

Merged

neutrinoceros mentioned this pull request Nov 6, 2025

#8651: Add version 7.0 docstring to methods for docs #18840

Closed

1 task

	if hasattr(votable, "_get_version_checks"):
	if hasattr(votable, "config"):

Conversation

timj commented Jun 28, 2025

Description

Uh oh!

github-actions Bot commented Jun 28, 2025

Uh oh!

timj Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

tomdonaldson Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

timj Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

timj commented Jun 28, 2025

Uh oh!

pllim Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

timj Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

bsipocz Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

pllim commented Jun 30, 2025

Uh oh!

timj commented Jun 30, 2025

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

tomdonaldson Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

timj commented Jul 11, 2025

Uh oh!

timj commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timj commented Jul 11, 2025

Uh oh!

timj commented Jul 11, 2025

Uh oh!

timj commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timj commented Jul 11, 2025 •

edited

Loading

timj commented Jul 11, 2025 •

edited

Loading