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

Asdf-Support #708

Open
wants to merge 30 commits into
base: asdf-support
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
39faa60
initial commit
ViciousEagle03 May 20, 2024
9644bbb
version alignment
ViciousEagle03 May 20, 2024
af7dccb
Remove method to remove any delay and
ViciousEagle03 May 22, 2024
67b8805
Update tag and
ViciousEagle03 May 22, 2024
b62ac94
enable schema test
ViciousEagle03 May 22, 2024
e2928ea
revert small change
ViciousEagle03 May 22, 2024
c2b240f
Add the converters for ExtraCoords and TimeTableCoordinate class
ViciousEagle03 May 30, 2024
94f2065
Add ExtraCoords and TimeTableCoord Schema
ViciousEagle03 May 30, 2024
cc65cc3
Update manifest and entry_point.py
ViciousEagle03 May 30, 2024
5ffb493
Add the QuantityTableCoordinate and SkyCoordTableCoordinate converters
ViciousEagle03 May 31, 2024
695f8f6
Add the schema for QuantityTableCoordinate and SkyCoordTableCoordinate
ViciousEagle03 May 31, 2024
9e38352
Update manifest and entry_point.py
ViciousEagle03 May 31, 2024
a9c5868
Add validation for schema and manifests
ViciousEagle03 Jun 4, 2024
835c5c7
Merge branch 'asdf-support' into asdf_basic_support
Cadair Jun 5, 2024
3bb70e4
pre-commit
Cadair Jun 5, 2024
04409b5
Update the tox.ini and CI workflow
ViciousEagle03 Jun 5, 2024
76cc27e
Update schemas
ViciousEagle03 Jun 5, 2024
3ea5e65
Add the converter and schema for GlobalCoords
ViciousEagle03 Jun 17, 2024
a31f5fc
Update converters
ViciousEagle03 Jun 17, 2024
f313395
Update entry_points,schemas and manifest
ViciousEagle03 Jun 17, 2024
3d9d9e8
minor change
ViciousEagle03 Jun 19, 2024
d031c35
lowercase schema URIs and use wildcards
ViciousEagle03 Jun 21, 2024
171dcd6
Add tests and GWCS objects
ViciousEagle03 Jul 1, 2024
0c7135f
merge from upstream/asdf-support
ViciousEagle03 Jul 4, 2024
0a84f8b
revert small change
ViciousEagle03 Jul 4, 2024
cd31d95
apply suggestions from code review
ViciousEagle03 Jul 6, 2024
7c04d35
Remove mesh as a property validator
ViciousEagle03 Jul 11, 2024
266f88e
Update the dependencies version
ViciousEagle03 Jul 11, 2024
67358b4
Style changes and add warnings to NDCube converter
ViciousEagle03 Jul 19, 2024
6e7a338
env name update
ViciousEagle03 Jul 19, 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
Empty file added ndcube/asdf/__init__.py
Empty file.
30 changes: 30 additions & 0 deletions ndcube/asdf/converters/ndcube_converter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from asdf.extension import Converter
import numpy as np


class NDCubeConverter(Converter):
tags = ["tag:sunpy.org:ndcube/ndcube/NDCube-*"]

@property
def types(self):
ViciousEagle03 marked this conversation as resolved.
Show resolved Hide resolved
from ndcube.ndcube import NDCube
return [NDCube]

def select_tag(self, obj, tags, ctx):

Choose a reason for hiding this comment

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

I would leave this out for now. As you've noted in the comments (which are very helpful!) the ordering here can sometimes lead to confusion. The tag versions can be managed by versioning the extension (in different manifests) so that for a given extension there is only one tag version (and we won't need a select_tag).

# Sort the tags in reverse alphabetical order and pick the first (i.e.
# the one with the highest version). This assumes that all the tags for
# this converter are named the same other than the version number.
tags = list(sorted(tags, reverse=True))
return tags[0]

def from_yaml_tree(self, node, tag, ctx):
from ndcube import ndcube
data = np.asanyarray(node["data"])
ViciousEagle03 marked this conversation as resolved.
Show resolved Hide resolved
wcs = node["wcs"]
return(ndcube.NDCube(data, wcs))

def to_yaml_tree(self, ndcube, tag, ctx):
node = {}
node["data"] = np.asarray(ndcube.data)
ViciousEagle03 marked this conversation as resolved.
Show resolved Hide resolved
node["wcs"] = ndcube.wcs
return node
40 changes: 40 additions & 0 deletions ndcube/asdf/entry_points.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
"""
This file contains the entry points for asdf.
"""
import importlib.resources as importlib_resources
from asdf.extension import ManifestExtension
from asdf.resource import DirectoryResourceMapping


def get_resource_mappings():
"""
Get the resource mapping instances for myschemas
and manifests. This method is registered with the
asdf.resource_mappings entry point.

Returns
-------
list of collections.abc.Mapping
"""
from ndcube.asdf import resources
resources_root = importlib_resources.files(resources)
return [

Check warning on line 21 in ndcube/asdf/entry_points.py

View check run for this annotation

Codecov / codecov/patch

ndcube/asdf/entry_points.py#L19-L21

Added lines #L19 - L21 were not covered by tests
DirectoryResourceMapping(
resources_root / "schemas", "asdf://sunpy.org/ndcube/schemas/"),
DirectoryResourceMapping(
resources_root / "manifests", "asdf://sunpy.org/ndcube/manifests/"),
]


def get_extensions():
"""
Get the list of extensions.
"""
from ndcube.asdf.converters.ndcube_converter import NDCubeConverter

Check warning on line 33 in ndcube/asdf/entry_points.py

View check run for this annotation

Codecov / codecov/patch

ndcube/asdf/entry_points.py#L33

Added line #L33 was not covered by tests

ndcube_converters = [NDCubeConverter()]
_manifest_uri = "asdf://sunpy.org/ndcube/manifests/ndcube-0.1.0"

Check warning on line 36 in ndcube/asdf/entry_points.py

View check run for this annotation

Codecov / codecov/patch

ndcube/asdf/entry_points.py#L35-L36

Added lines #L35 - L36 were not covered by tests

return [

Check warning on line 38 in ndcube/asdf/entry_points.py

View check run for this annotation

Codecov / codecov/patch

ndcube/asdf/entry_points.py#L38

Added line #L38 was not covered by tests
ManifestExtension.from_uri(_manifest_uri, converters=ndcube_converters)
]
10 changes: 10 additions & 0 deletions ndcube/asdf/resources/manifests/ndcube-0.1.0.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
%YAML 1.1
---
id: asdf://sunpy.org/ndcube/manifests/ndcube-0.1.0
extension_uri: asdf://sunpy.org/extensions/ndcube-0.1.0
title: NDCube ASDF Manifest
description: ASDF schemas and tags for NDCube classes.

tags:
- tag_uri: "tag:sunpy.org:ndcube/ndcube/NDCube-0.1.0"
schema_uri: "asdf://sunpy.org/ndcube/schemas/NDCube-0.1.0"
21 changes: 21 additions & 0 deletions ndcube/asdf/resources/schemas/NDCube-0.1.0.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
%YAML 1.1
---
$schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
id: "asdf://sunpy.org/ndcube/schemas/NDCube-0.1.0"

title: |
Represents the ndcube NDCube object

description:
Represents the ndcube NDCube object

type: object
properties:
data:
tag: "tag:stsci.edu:asdf/core/ndarray-1.0.0"
ViciousEagle03 marked this conversation as resolved.
Show resolved Hide resolved
wcs:
$ref: "http://stsci.edu/schemas/gwcs/wcs-1.1.0"
ViciousEagle03 marked this conversation as resolved.
Show resolved Hide resolved

required: [data , wcs]
allowAdditionalProperties: False
Copy link
Member

Choose a reason for hiding this comment

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

If I subclass NDCube and want to write my own schema for my subclass with additional properties does this get in the way? @braingram

Choose a reason for hiding this comment

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

I think so. A typical way to "subclass" a schema is to $ref the parent schema in a allOf. Using allowAdditionalProperties: False here would prevent the sub-schema from adding properties (because any addition would cause the parent schema validation to fail, causing the allOf to fail).

...
6 changes: 6 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ exclude = ["ndcube._dev*"]
[tool.setuptools_scm]
write_to = "ndcube/_version.py"

[project.entry-points."asdf.resource_mappings"]
ndcube = "ndcube.asdf.entry_points:get_resource_mappings"

[project.entry-points."asdf.extensions"]
ndcube = "ndcube.asdf.entry_points:get_extensions"

Choose a reason for hiding this comment

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

Would you also enable schema tests for the new schemas? Here's the configuration for asdf-astropy:
https://github.com/astropy/asdf-astropy/blob/1b286108042c1201199e9931cd2a5b52cc57bdce/pyproject.toml#L121-L122

I am not familiar with the ndcube tox and github actions CI but would you look into modifying whatever is necessary there to get the schema tests running for the PR? Let me know if there are more details or anything I can do to help with this. Also, perhaps one of the other maintainers has a sense for how these schema tests might be integrated in the CI.

Copy link
Member Author

@ViciousEagle03 ViciousEagle03 May 22, 2024

Choose a reason for hiding this comment

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

I have enabled the schema test, @Cadair could you please verify if the configuration is correct?
I followed this to enable the tests. Thanks!

Choose a reason for hiding this comment

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

I don't think this has allowed the schema tests to run in the CI. I think it's related to asdf-format/asdf#1756

The core issue is that the CI run is testing files in the installed package but is running from a temporary directory. Take this test as an example:

../../.tox/py312/lib/python3.12/site-packages/ndcube/extra_coords/tests/test_extra_coords.py::test_empty_ec PASSED

The path to the file contains a bunch of stuff that is very specific to that test run (py312, tox, etc) and this causes the asdf pytest plugin to ignore the schema files since they appear at a similar path which doesn't match the asdf_schema_root.

As there are test configurations where we might expect the schema tests to fail (like the oldest deps job, see this conversation for more details) we may want to consider adding a new CI job (or jobs) that test just the schemas. Minimally they could be tested against the newest released asdf and perhaps they could also be tested against development versions of asdf and the dependent schema packages. Ignoring the development testing at the moment I think this would involve:

  • add a tox factor that runs just the schema tests (defining asdf_schema_root, providing a path to the schemas, not running in a temporary directory)
  • enabling this factor in the CI workflow

Copy link
Member Author

@ViciousEagle03 ViciousEagle03 Jun 5, 2024

Choose a reason for hiding this comment

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

I've included a separate environment for schema testing in tox and created a workflow for it. Should I add a separate job for the schema tests as well? Please let me know. (P.S. I got a bit carried away with the workflow concept.)
The schema tests(now enabled) in the CI now doesn't run from a .tmp directory:

../../ndcube/asdf/resources/manifests/ndcube-0.1.0.yaml .                [ 16%]
110
../../ndcube/asdf/resources/schemas/ExtraCoords-0.1.0.yaml .             [ 33%]
111
../../ndcube/asdf/resources/schemas/NDCube-0.1.0.yaml .                  [ 50%]
112
../../ndcube/asdf/resources/schemas/QuantityTableCoordinate-0.1.0.yaml . [ 66%]
113
                                                                         [ 66%]
114
../../ndcube/asdf/resources/schemas/SkyCoordTableCoordinate-0.1.0.yaml . [ 83%]
115
                                                                         [ 83%]
116
../../ndcube/asdf/resources/schemas/TimeTableCoordinate-0.1.0.yaml .     [100%]

Choose a reason for hiding this comment

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

The workflow and tox changes you've included so far look good enough to me. I don't think adding a separate job is necessary (but please let me know if I'm missing something that would provide). Thanks!

[tool.towncrier]
package = "ndcube"
filename = "CHANGELOG.rst"
Expand Down
Loading