chanzuckerberg / cryoet-data-portal-backend

CryoET Data Portal API server & ingestion scripts
MIT License
1 stars 2 forks source link

Update table and column descriptions #188

Closed andy-sweet closed 2 months ago

andy-sweet commented 2 months ago

Relates to https://github.com/chanzuckerberg/cryoet-data-portal/issues/894

Description

This adds a migration that alters table and column comments. Some of the older comments were incorrect, inconsistent, had typos, or were missing. The new comments mostly come from the manually maintained docstrings in the Python client from https://github.com/chanzuckerberg/cryoet-data-portal .

The motivation for doing this to ensure we have GraphQL scheme comments/descriptions, which can then be used to automatically generate correct docstrings for the Python client (as in https://github.com/chanzuckerberg/cryoet-data-portal/pull/910).

I have tested this locally in conjunction with https://github.com/chanzuckerberg/cryoet-data-portal/pull/910 , which now pulls in the updated comments when using the locally hosted GraphQL server.

andy-sweet commented 2 months ago

This is a draft PR because I am unsure how to successfully define a comment/description on the relationships.

Based on the Hasura docs, it looks like providing a comment for the relationship in the table's metadata YAML file should work. I did this for a few files here to illustrate. But seems to be ignored with the default "An array relationship" or "An object relationship" being used instead. I also tried a few quick guesses to fix this (e.g. quoting the comment string, using description instead of comment), but with no success.

The Hasura docs also mention set_relationship_comment but I'm unsure where I would define that to take an effect.

For local deployment, I am simply running make clean then make init.

The Hasura docs also mention hasura metadata [apply|reload], but I'm assuming those are used for live updates and should be handled by make init.

manasaV3 commented 2 months ago

Currently hasura manages the migrations application. When we do a deploy to an environment, hasura should automatically apply the new migrations that have been added since the timestamp it was last updated. The unix timestamp here maps to the folder prefix.

andy-sweet commented 2 months ago

This is a draft PR because I am unsure how to successfully define a comment/description on the relationships.

Thanks to @daniel-ji for finding a related existing issue: https://github.com/hasura/graphql-engine/issues/9600

Which suggests that Hasura does not currently use the various ways to encode a comment on a relationship, in the resulting GraphQL schema.

For future reference, I think I found the direct implementation detail that causes this: https://github.com/hasura/graphql-engine/blob/6c9dad1786a25ff5c59b2d10a4c21a183d4397c8/server/src-lib/Hasura/NativeQuery/Schema.hs#L320

which in turn occurs because RelInfo (Relationship Information) has no way to store the comment/description from the table metadata or from the comment of a particular backend. I stopped back-tracking through the code there because that suggests an implementation for this feature is at least non-trivial.

andy-sweet commented 2 months ago

Since Hasura does not seem to use the relationship comments, I removed them for now. The workaround in https://github.com/chanzuckerberg/cryoet-data-portal/pull/910/ is to generate the comment from the types involved in the relationship.

andy-sweet commented 2 months ago

@manasaV3 : Just wanted to check that there's nothing wrong with throwing all the comment updates into a single migration with the generic name of alter_comments.

andy-sweet commented 2 months ago

@manasaV3 : this still needs an approval. Feel free to add line new comments or suggestions if there are more required additions or corrections.