mapping-commons / sssom-py

Python toolkit for SSSOM mapping format
https://mapping-commons.github.io/sssom-py/index.html#
MIT License
49 stars 12 forks source link

Deprecate `get_metadata_and_prefix_map` #486

Closed cthoyt closed 7 months ago

cthoyt commented 7 months ago

This function used to be used in OAK's lexical matcher pipeline, but has since been removed. Now, the only place it's (potentially mis)used is in the MONDO ingest pipeline (see https://github.com/search?q=get_metadata_and_prefix_map+-is%3Afork+-user%3Amapping-commons&type=code)

This PR deprecates it, and we can remove it in a future version.

cthoyt commented 7 months ago

Here my question is: if we make this method private, do we still have a utility method to load a externally defined sssom metadata file?

No, we don't. Since external metadata is a YAML file, I don't think we even need one, since you can do one of the following:

  1. Use the SSSOM-py parsers that handle these files implicitly
  2. import a standard YAML processor if you want to work with it directly

The issue with having a built in function is it has super confusing semantics about how it combines metadata already inside the YAML file with some other metadata given ad hoc. I don't think we should have a function that does this and ask downstream users to implement their own merging, since it will always have different needs.

joeflack4 commented 7 months ago

@matentzn Haven't put much thought in it yet, but I think it's possible that @cthoyt may be right. I can probably refactor mondo-ingest to do what it needs to do without relying on sssom-py for this.

matentzn commented 7 months ago

I don't think we should have a function that does this and ask downstream users to implement their own merging, since it will always have different needs.

Fair enough. We can provide a wrapper of sssom parse later when a user requests it.

@matentzn Haven't put much thought in it yet, but I think it's possible that @cthoyt may be right. I can probably refactor mondo-ingest to do what it needs to do without relying on sssom-py for this.

Thanks for looking into this.

cthoyt commented 7 months ago

@matentzn I forgot an import, can you please approve this again