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

make type slot *not* required #89

Closed
wdduncan opened this issue Jan 24, 2022 · 9 comments
Closed

make type slot *not* required #89

wdduncan opened this issue Jan 24, 2022 · 9 comments
Assignees
Milestone

Comments

@wdduncan
Copy link
Contributor

In order for change sheets to work correctly, the entity being updated needs to have value in its type slot. So, we need to require the type slot.

@dehays Do you thinktype needs to be required for all class in the schema? I think "yes", but we may want to discuss more.

For reference, this issue came about as a result of #88

cc @cmungall @turbomam @sujaypatil96

@wdduncan wdduncan added this to To do in NMDC February 2022 Sprint via automation Jan 24, 2022
@ssarrafan ssarrafan added this to the Sprint 11 milestone Feb 1, 2022
@wdduncan
Copy link
Contributor Author

wdduncan commented Feb 28, 2022

Update:
In the latest changes, the type slot in the schema has been deprecated. So, the type slot should not be required.
See this thread for discussion concerning deprecation of type slot: microbiomedata/nmdc-schema#248

In fact, I have some code in change sheets where I'll need to un-require the type.

@ssarrafan
Copy link

Moving this to March per @wdduncan as he plans to close before the end of the week

Note from Bill:
HI Set. Here is an update:
microbiomedata/nmdc-schema#134
This is an ongoing issue that will need to be passed on to Mark or Sujay (not sure which).
#89
I updated the comment on this. I should be able to get the change sheet edit done before I leave.
microbiomedata/nmdc-schema#195
I am working with Sujay on this. I should be able to close before the week’s end.
#46
This is an ongoing issue that will need to be passed on to Mark or Sujay (not sure which).
microbiomedata/nmdc-server#555
This is a lot of conversation in this thread. But, it looks like Mark has taken this one over.

@ssarrafan
Copy link

@turbomam @dehays @cmungall this was an old issue still assigned to Bill. I re-assigned it to @turbomam. Should this issue be closed or moved to a future sprint?

@wdduncan
Copy link
Contributor Author

Unfortunately, I was not able to fix this issue before leaving. In the file metadata.py here is the line that needs to be fixed:

raise Exception("Cannot determine the type of class for ", id_)

It may suffice to simply set class_name = "" when a type could not be determined. However, this needs to be tested. There are couple of testing notebooks I set up for running tests:

changesheets-testing.ipynb
changesheets-example.ipynb

You may need to enlist @dehays and @dwinston for help explaining how changesheets work.

@ssarrafan
Copy link

Thanks @wdduncan for the update from Florida!
@turbomam wants to continue this to April so I'll move it to the April sprint

@ssarrafan ssarrafan removed this from To do in NMDC March 2022 Sprint Mar 31, 2022
@ssarrafan ssarrafan added this to To do in NMDC April 2022 Sprint via automation Mar 31, 2022
@ssarrafan ssarrafan modified the milestones: Sprint 12, Spring 13 Mar 31, 2022
@turbomam
Copy link
Collaborator

I feel like it should be super straightforward to set type's required attribute to false, but @wdduncan's comment about changing a line in the autogenerated metadata.py makes me think I'm missing the point. Also, I certainly don't have a good sense of all of the ways an edit like this could affect all of the systems that consume that are guided by the nmdc-schema. Like parts of the portal? Or some ingest steps?

type has never been are required slot for biosamples in the DH templates.

Also, we have been recently talking about "overloading" the biosample class by instantiating it for material entities in a sample processing chain. Does that touch on this issue at all?

@dwinston
Copy link
Collaborator

dwinston commented Apr 18, 2022

Taking a quick look at this, it seems that collection_name is guaranteed to be set at the point of the changesheet processing code that seeks to set class_name, and these two values are one-to-one and connected in the json schema, e.g. the 'biosample_set' collection is known to be a list of objects of type 'Biosample'. So, I think type can be deduced unambiguously.

@wdduncan
Copy link
Contributor Author

The type slot is now deprecated:
https://microbiomedata.github.io/nmdc-schema/attribute_value_type/#slot-type-deprecated

So, I think the goal was to remove any need for it in the code base.

@dwinston
Copy link
Collaborator

fixed via 54439bf

NMDC April 2022 Sprint automation moved this from To do to Done Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants