DM-54017: Use Felis for loading the APDB schemas#57
Conversation
bsmartradio
force-pushed
the
tickets/DM-54017
branch
from
3d0ce27 to
3815d37
Compare
gpdf
left a comment
gpdf
left a comment
There was a problem hiding this comment.
In commit b04b4ce, "Update schema to v11.0 and swap to Felis", a new schema is combined on the same commit with a new schema. We would encourage decoupling these so that it's possible to validate the new avsc-generation using Felis first against an existing one, and then separately committing the result of the schema generation in a different PR.
I also have some comments on the units in the generated schema, which probably need to be fed back to changes in the upstream Felis, where I do see that the "d" units are missing on the attributes corresponding to the comments below.
| ] | ||
| }, | ||
| { | ||
| "doc": "Time when this record was generated, expressed as Modified Julian Date, International Atomic Time.", |
| "type": "double" | ||
| }, | ||
| { | ||
| "doc": "Time when this record was marked invalid, expressed as Modified Julian Date, International Atomic Time.", |
| "type": "long" | ||
| }, | ||
| { | ||
| "doc": "Processing time when validity of this diaObject starts, expressed as Modified Julian Date, International Atomic Time.", |
gpdf
left a comment
gpdf
left a comment
There was a problem hiding this comment.
In commit b04b4ce, "Update schema to v11.0 and swap to Felis", a new schema is combined on the same commit with a new schema. We would encourage decoupling these so that it's possible to validate the new avsc-generation using Felis first against an existing one, and then separately committing the result of the actual schema generation in a different PR.
I also have some comments on the units in the generated schema, which probably need to be fed back to changes in the upstream Felis YAML rather than being fixed on this ticket. I'll submit a bug ticket to Jira for that.
Unfortunately, we cannot separate the Felis change and the schema change. We use Felis in updatSchema.py, which builds from the current sdm_schemas. The schema change is needed because a change was made a while back to updating fields were non-nullable. It wasn't caught that the change would have also required a major schema update. This currently does not cause issues, but still is a needed change. Additionally, the missing units will require a change to sdm_schemas, as we build everything from there. I can open a branch on sdm_schemas to make this change, assuming it needs to be present in all the documentation. I'd just like to confirm that that is what you are asking before making the changes there. |
bsmartradio
force-pushed
the
tickets/DM-54017
branch
from
3815d37 to
bc76fe4
Compare
gpdf
left a comment
gpdf
left a comment
There was a problem hiding this comment.
OK, I see that it's not realistic at this point to deploy things the way that I'd have preferred, in two steps.
I would still have preferred the code change and the new v11 schema to be on separate commits, for a clearer history, though.
If you have time to do that today, I'd appreciate it; otherwise you can proceed. We can fix up the units soon as another change. Please go ahead and make an sdm_schemas PR for that.
Thanks!
bsmartradio
force-pushed
the
tickets/DM-54017
branch
from
bc76fe4 to
3dfbd76
Compare
bsmartradio
force-pushed
the
tickets/DM-54017
branch
from
3dfbd76 to
a0d6d7a
Compare
Schema update changes all nullable int fields to non-nullable
bsmartradio
force-pushed
the
tickets/DM-54017
branch
from
a0d6d7a to
d00371c
Compare
|
Did this merge break ap_association unit tests? My Jenkins builds seem to be failing, and I don't think it's due to my changes: https://ci.lsst.cloud/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/8870/tests |
|
Looks like weekly build will be broken due to this, but it may be too late to revert. |
4 participants
No description provided.