Skip to content

Add w3curie type and edit def of curie to be less restrictive #221

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ialarmedalien
Copy link

@ialarmedalien ialarmedalien commented Mar 27, 2025

Fixes linkml/linkml#258.

Adding in the new w3curie type.

I tried to update the repo to use py3.12 with the current versions of linkml, linkml-runtime, and pyyaml so that I could regenerate the project files. Run make gen-project gave the following output:

# for all the files in the schema folder, run the gen-python command and output the result to the top
# level of the project.  In other repos, we'd include mergeimports=True, but we don't do that with
# linkml-model.
ValueError:  Unknown argument: slot_usage = {'dimensions': {'equals_number': 1}}
ValueError:  type "any_number" must declare a type base or parent (typeof)
cp staging/*.py linkml_model
poetry run gen-project -d staging --config-file gen_project_config.yaml linkml_model/model/schema/meta.yaml
Traceback (most recent call last):
[...snip a load of callback lines...]
ValueError: The ifabsent value `default_range` of the `range` slot could not be processed
make: *** [gen-project] Error 1

Looks like the various project generators, etc., have become more restrictive since linkml-model was last updated.

ETA: just seen the series of PRs to update the model repo. I can rebase this PR and generate the new files once the deps and other bits and pieces are fixed and merged.

@ialarmedalien ialarmedalien marked this pull request as ready for review March 27, 2025 19:13
@ialarmedalien
Copy link
Author

@cmungall @sujaypatil96 and anyone else who is interested -- please add any comments as it'd be great to get this merged!

@Silvanoc
Copy link

LGTM.

We could then adapt some of the validations to be strict on w3curie.

@Silvanoc
Copy link

Silvanoc commented Jun 18, 2025

Would it be possible to have something like w3safecurie too as a subtype of curie? That way we could have something like URIorSafeCURIE too.

[Update]: thanks @dalito for the link

@dalito
Copy link
Member

dalito commented Jun 19, 2025

In this repo I cannot help with approving or merging but I also support Silvano´s request for the other CURIE Datatypes.

@ialarmedalien
Copy link
Author

I don't have merge permissions -- @cmungall @sierra-moxon can you approve/merge?

@Silvanoc
Copy link

I don't have merge permissions -- @cmungall @sierra-moxon can you approve/merge?

@ialarmedalien I can merge, but the (undocumented) policy says that only the core-team is allowed to merge. And I'm not part of it.

I started a discussion to get this kind of policies, workflows and processes documented. But up to now I didn't get any resonance in the community 🤷

@ialarmedalien
Copy link
Author

I'm with you on the lack of clarity on permissions that you talk about in your discussion! I'd guess there hasn't been much response to the issue because (a) discussions are hidden away from the main issue tracker / PRs, and (b) it's a very difficult question to answer...

@Silvanoc
Copy link

I'm with you on the lack of clarity on permissions that you talk about in your discussion!

Then vote it up 😉

I'd guess there hasn't been much response to the issue because (a) discussions are hidden away from the main issue tracker / PRs, and (b) it's a very difficult question to answer...

That's why I raise the point whenever I find a fitting situation within the scope of a PR or issue, like in this case.

Additionally, people give "thumbs up" instead of "votes". And discussion lists show votes, but not emojis 🤷

@nlharris
Copy link
Contributor

@Silvanoc good point about "thumbs up" vs. "votes". I think for a thumbs-up to count as a vote, it has to be on the issue description (i.e., the first "comment", as opposed to any subsequent comments) - is that correct?

Regarding this PR, I realize you are eager to get it merged. However, we are extra-careful about changes that affect the metamodel. This PR has no automated tests to make sure it doesn't break anything else, and it's also not clear whether anyone manually tested how this change affects things when imported into an existing LinkML model repository (such as Biolink) or any downstream repo in the LinkML ecosystem. To avoid potential problems caused by merging this PR, we need people who are deeply familiar with the metamodel and the whole codebase to review it. (This would of course be easier if there were tests included.)

I hear you on the frustration about lack of clarity in the process for reviewing and merging PRs. This is something that many open-source projects run into as they grow. Thank you for opening Discussion #2518 and including examples of some other open-source projects' contribution protocols. We will definitely look into those.

@ialarmedalien
Copy link
Author

@nlharris Not sure what kind of tests should be added -- the PR registers a new data type, derived from existing data types, to the data model; it does not add any functions or methods, which would require tests. There are no extant tests for this part of the model that new tests could be modelled on. I'm happy to add some if you want to suggest what they should do.

@Silvanoc
Copy link

@Silvanoc good point about "thumbs up" vs. "votes". I think for a thumbs-up to count as a vote, it has to be on the issue description (i.e., the first "comment", as opposed to any subsequent comments) - is that correct?

Apparently "thumbs up" are never counted as "votes" in the discussions view. Discussion #2518 has 3 votes (one of them mine) and 3 thumb-ups (none of them mine), so "thumb-up" =/= "vote". And the discussion view does not add them up, but only show the votes (3).

Regarding this PR, I realize you are eager to get it merged. However, we are extra-careful about changes that affect the metamodel. This PR has no automated tests to make sure it doesn't break anything else, and it's also not clear whether anyone manually tested how this change affects things when imported into an existing LinkML model repository (such as Biolink) or any downstream repo in the LinkML ecosystem. To avoid potential problems caused by merging this PR, we need people who are deeply familiar with the metamodel and the whole codebase to review it. (This would of course be easier if there were tests included.)

I agree with @ialarmedalien that there is probably no need for any tests to be added. Being a derived type, tests applicable to the type parent applies also here. Once we extend validation to really differentiate this subtype from its parent, we will need tests.

I hear you on the frustration about lack of clarity in the process for reviewing and merging PRs. This is something that many open-source projects run into as they grow. Thank you for opening Discussion #2518 and including examples of some other open-source projects' contribution protocols. We will definitely look into those.

OSS projects typically start opening the source code, but are not open communities. At that point there might not be a real need to processes, as long as the community knows some otherwise established or common processes. But OSS projects eventually become also open communities. And at that point openly documented processes become important for a smooth community interaction. IMO LinkML has reached that point for a long time now and should really document at least the basics.

@ialarmedalien
Copy link
Author

OSS projects typically start opening the source code, but are not open communities. At that point there might not be a real need to processes, as long as the community knows some otherwise established or common processes. But OSS projects eventually become also open communities. And at that point openly documented processes become important for a smooth community interaction. IMO LinkML has reached that point for a long time now and should really document at least the basics.

Agree completely. As @Silvanoc alluded to in an earlier comment, there seems to be a "core" LinkML developer team, but it's not clear who is part of that team beyond Chris M and Sierra. Having established and documented procedures for PR triage would also be beneficial for maintaining positive sentiment in the community.

@sierra-moxon
Copy link
Member

sierra-moxon commented Jun 30, 2025

@ialarmedalien - I understand from this PR that the new type is wanted and warranted. And I like the new type.

Has this new type been incorporated into local linkml-runtime and linkml branches, and run through the tests in those repos?

From the tests in this repo: In the case of LinkML model, this set includes the model itself. When making changes to the LinkML model, replace these files with those that you have changed manually. https://github.com/linkml/linkml-model/blob/main/tests/input/types.yaml

As a core dev, my next steps with this PR would be to:

  • regenerate python, pydantic, jsonschema from this model change and using this core model to regenerate prod-models (like biolink) serialization of this because I know that those serializations are really important to existing models.
  • try to migrate my changes to the input dir here and run through existing tests and document hard parts somewhere (or come to a dev call with them).
  • if the heavy-weight testing I had to do to confirm this model change could be improved upon, and I had time, I would make tickets. If I didn't have time, I would put a note in the PR to the reviewer about what I did.
  • I would try to get the 3.12, 3.13 upgrade done before adding functionality that can't be incorporated downstream. (noting in this PR, that this is ongoing work being done in another PR).

I'll get started on this, I've added myself as a reviewer.

@sierra-moxon sierra-moxon self-requested a review June 30, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a pure CURIE type
5 participants