DM-42935: Refactor tap module to use the Pydantic data model #49

JeremyMcCormick · 2024-03-25T21:28:35Z

The tap module is updated to use the Pydantic data model, instead of the raw YAML. The "load-tap" command in the command line interface was also updated to use the Pydantic schema object. The PYLD transformations were removed from the cli function, as they seemed to be unnecessary. A few changes were made to the Pydantic data model to support setting of reasonable defaults. A "votable_datatype" annotation was added to Colum. The Column datatype was changed to return the enum instead of a string, as this setting seemed to confuse mypy. Tests were changed to conform to the new tap module and the minor changes to the data model. Testing showed that the schema was loaded correctly into a live PostgreSQL database.

Diff the DDL output for the TAP_SCHEMA from main (production) with this branch.

codecov · 2024-03-25T21:32:30Z

Codecov Report

Attention: Patch coverage is 98.73418% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.58%. Comparing base (0d31a49) to head (17ca2bb).
Report is 36 commits behind head on main.

❗ Current head 17ca2bb differs from pull request most recent head cbad0f1. Consider uploading reports for the commit cbad0f1 to get more accurate results

Files	Patch %	Lines
python/felis/tap.py	98.36%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   91.14%   91.58%   +0.43%     
==========================================
  Files          21       21              
  Lines        2225     2198      -27     
  Branches      468      458      -10     
==========================================
- Hits         2028     2013      -15     
+ Misses        131      125       -6     
+ Partials       66       60       -6

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

andy-slac

Looks great, a couple of minor comments.

python/felis/tap.py

JeremyMcCormick · 2024-04-03T20:30:17Z

@gpdf Following our discussion of validating the new tap module which uses Pydantic, I have attached below the existing SQL output from main and that from the updated module. They are exactly the same according to diff.

dp02_dc2_tap_main.sql.txt
dp02_dc2_tap_new.sql.txt

JeremyMcCormick · 2024-04-19T17:26:47Z

@gpdf As per discussion today, I reverted addition of the tap:schema_index field on the Pydantic data model, which should have no effect on the TAP_SCHEMA output. The existing scripts within sdm_schemas handle this in a way which does not use the datamodel module.

JeremyMcCormick · 2024-04-19T17:35:01Z

@andy-slac I invalidated your review, as there are some fix-ups needed here related to setting the TAP_SCHEMA index. I'll have @gpdf review this in its final form.

JeremyMcCormick · 2024-04-19T19:02:11Z

python/felis/datamodel.py

+    votable_utype: str | None = Field(None, alias="votable:utype")
+    """The VOTable utype (usage-specific or unique type) of the object."""
+
+    @model_validator(mode="after")


These changes are a fix to the Pydantic model_validator as it was using the wrong function signature.

The tap module is updated to use the Pydantic data model, instead of the raw YAML. The "load-tap" command in the command line interface was also updated to use the Pydantic schema object. The PYLD transformations were removed from the cli function, as they seemed to be unnecessary. A few changes were made to the Pydantic data model to support setting of reasonable defaults. A "votable_datatype" annotation was added to Colum. The Column datatype was changed to return the enum instead of a string, as this setting seemed to confuse mypy. Tests were changed to conform to the new tap module and the minor changes to the data model. Testing showed that the schema was loaded correctly into a live PostgreSQL database.

gpdf

Looks good in general. Please make the one change about the handling of missing table_index values.

gpdf · 2024-04-23T19:55:00Z

python/felis/datamodel.py

@@ -391,7 +393,7 @@ class Table(BaseObject):
    primary_key: str | list[str] | None = Field(None, alias="primaryKey")
    """The primary key of the table."""

-    tap_table_index: int | None = Field(None, alias="tap:table_index")
+    tap_table_index: int = Field(0, alias="tap:table_index")


Please retain the ability for the model to distinguish between "missing" and zero table_index values. We may wish to make the handling more sophisticated in the (near) future.

I think column_index still allows None anyway, right?

gpdf · 2024-04-23T19:56:17Z

python/felis/tap.py

-        table.table_index = int(table_obj.get("tap:table_index", 0))
+        table.utype = table_obj.votable_utype
+        table.description = table_obj.description
+        table.table_index = table_obj.tap_table_index


This line would have to be changed as well, perhaps to something like

table.table_index = 0 if table_obj.tap_table_index is None else table_obj.tap_table_index

Make the table index None by default instead of 0 and update the logic in the tap module accordingly.

gpdf

Looks good

JeremyMcCormick requested a review from andy-slac March 25, 2024 21:28

JeremyMcCormick force-pushed the tickets/DM-42935 branch 2 times, most recently from 51c0aab to db1de2d Compare March 29, 2024 17:37

andy-slac approved these changes Mar 29, 2024

View reviewed changes

python/felis/tap.py Outdated Show resolved Hide resolved

python/felis/tap.py Show resolved Hide resolved

python/felis/tap.py Outdated Show resolved Hide resolved

JeremyMcCormick force-pushed the tickets/DM-42935 branch from db1de2d to 37b235e Compare April 3, 2024 20:27

JeremyMcCormick requested a review from gpdf April 3, 2024 21:16

JeremyMcCormick force-pushed the tickets/DM-42935 branch from 37b235e to 17ca2bb Compare April 3, 2024 21:51

JeremyMcCormick marked this pull request as draft April 8, 2024 23:07

JeremyMcCormick marked this pull request as ready for review April 12, 2024 00:43

JeremyMcCormick changed the title ~~Refactor tap module to use the Pydantic data model~~ DM-42935: Refactor tap module to use the Pydantic data model Apr 12, 2024

JeremyMcCormick force-pushed the tickets/DM-42935 branch from 17ca2bb to 95c017f Compare April 19, 2024 17:25

JeremyMcCormick requested review from andy-slac and removed request for andy-slac April 19, 2024 17:34

JeremyMcCormick force-pushed the tickets/DM-42935 branch from 95c017f to 8e8d967 Compare April 19, 2024 18:55

JeremyMcCormick commented Apr 19, 2024

View reviewed changes

JeremyMcCormick force-pushed the tickets/DM-42935 branch 2 times, most recently from f96594d to 8e8d967 Compare April 22, 2024 22:17

JeremyMcCormick added 2 commits April 22, 2024 17:34

Add option to set the schema index from the command line (DM-43683)

9d639fb

JeremyMcCormick force-pushed the tickets/DM-42935 branch from 8e8d967 to 9d639fb Compare April 22, 2024 22:34

gpdf requested changes Apr 23, 2024

View reviewed changes

Make table index None by default

cbad0f1

Make the table index None by default instead of 0 and update the logic in the tap module accordingly.

gpdf approved these changes Apr 23, 2024

View reviewed changes

gpdf mentioned this pull request Apr 23, 2024

DM-42935: Set the schema index using Felis command line argument lsst/sdm_schemas#206

Merged

JeremyMcCormick merged commit f31d1e4 into main Apr 23, 2024
9 checks passed

JeremyMcCormick deleted the tickets/DM-42935 branch April 23, 2024 22:14

This was referenced Apr 29, 2024

DM-43668: Remove pyld dependency and functionality #68

Merged

Remove old tap-schema script lsst/sdm_schemas#213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42935: Refactor tap module to use the Pydantic data model #49

DM-42935: Refactor tap module to use the Pydantic data model #49

JeremyMcCormick commented Mar 25, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

andy-slac left a comment

JeremyMcCormick commented Apr 3, 2024

JeremyMcCormick commented Apr 19, 2024

JeremyMcCormick commented Apr 19, 2024 •

edited

JeremyMcCormick Apr 19, 2024

gpdf left a comment

gpdf Apr 23, 2024

gpdf Apr 23, 2024

gpdf Apr 23, 2024

gpdf left a comment

DM-42935: Refactor tap module to use the Pydantic data model #49

DM-42935: Refactor tap module to use the Pydantic data model #49

Conversation

JeremyMcCormick commented Mar 25, 2024 • edited

codecov bot commented Mar 25, 2024 • edited

Codecov Report

andy-slac left a comment

Choose a reason for hiding this comment

JeremyMcCormick commented Apr 3, 2024

JeremyMcCormick commented Apr 19, 2024

JeremyMcCormick commented Apr 19, 2024 • edited

JeremyMcCormick Apr 19, 2024

Choose a reason for hiding this comment

gpdf left a comment

Choose a reason for hiding this comment

gpdf Apr 23, 2024

Choose a reason for hiding this comment

gpdf Apr 23, 2024

Choose a reason for hiding this comment

gpdf Apr 23, 2024

Choose a reason for hiding this comment

gpdf left a comment

Choose a reason for hiding this comment

JeremyMcCormick commented Mar 25, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

JeremyMcCormick commented Apr 19, 2024 •

edited