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

Fixes #2725 clm-cl.owl component edits #2763

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Caroline-99
Copy link
Collaborator

@Caroline-99 Caroline-99 commented Nov 15, 2024

clm-cl.owl component edits - This work was done under the supervision of @hkir-dev

  • Add step to makefile to pull content into components folder
  • Add import statement to cl-edit.owl

blocked by Cellular-Semantics/CellMark#9.

clm-cl.owl component edits
@Caroline-99 Caroline-99 linked an issue Nov 15, 2024 that may be closed by this pull request
2 tasks
@Caroline-99 Caroline-99 marked this pull request as draft November 15, 2024 09:56
@dosumis
Copy link
Contributor

dosumis commented Nov 17, 2024

Looks good, but maybe some unrelated edits here too - e.g. on taxon constraints?

Copy link
Collaborator

@gouttegd gouttegd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the addition of the clm-cl.owl component itself, no problem except that at some point we will need to make the downloading of the component conditional, so that it is only refreshed when a refresh is explicitly required and not when running the QC pipelines (#2644). But this is out of scope of this PR.

The real issue here is the overwriting of our custom QC workflow file (.github/workflows/qc.yml), which must be reverted.

env:
DEFAULT_BRANCH: master
run: cd src/ontology && make ROBOT_ENV='ROBOT_JAVA_ARGS=-Xmx6G' test IMP=false PAT=false MIR=false

- name: Reason over taxon constraints
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes to that file should not have been committed.

When running make update_repo, the ODK automatically updates a bunch of files, including the .github/workflows/qc.yml file. But in CL, we are running a customised version of the QC check (in which we additionally check for taxon constraint violations), so our own qc.yml file should not be replaced by the ODK-generated one.

So, after running make update_repo, all changes to .github/workflows/qc.yml should be reverted so as to leave our original file unchanged.

@@ -1,7 +1,7 @@
# ----------------------------------------
# Makefile for cl
# Generated using ontology-development-kit
# ODK Version: v1.5.3
# ODK Version: v1.5.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are running the ODK 1.5.2. This is not a problem here, but please note that CL’s QC and release workflows now require the ODK 1.5.3.

@@ -313,6 +313,14 @@ CELLXGENE_SUBSET_URL="https://raw.githubusercontent.com/hkir-dev/cellxgene-cell-
$(TEMPLATEDIR)/cellxgene_subset.tsv:
wget $(CELLXGENE_SUBSET_URL) -O $@

# CellMark reference subset
CLM_CL_URL="https://raw.githubusercontent.com/Cellular-Semantics/CellMark/main/clm-cl.owl"
$(TMPDIR)/clm-cl.owl:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in line with how we “manage“ the already existing components, so it’s fine for now.

But this will need to be changed at some point to uncouple the refreshing of components from the release and QC processes (#2644).

@gouttegd
Copy link
Collaborator

The real issue here is the overwriting of our custom QC workflow file (.github/workflows/qc.yml), which must be reverted.

Unfortunately it won’t be practical to do this starting from this PR, since the changes to qc.yml have been committed as part of a single commit with all the other changes.

Better to actually start anew in a completely distinct PR. I can take care of that.

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.

Add cellmark CL release artefact to CL components
4 participants