Cleaning up makefile #3028
Merged
Cleaning up makefile #3028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Purge the custom Makefile from all code that is commented out. Everything is preserved for eternity in the Git history, so there's no need to keep old stuff in the Makefile.
We are no longer using Travis for CI, so remove all traces of it.
The 'core.owl' product is no longer needed now. Its only purpose was to be merged with phenoscape-ext, which is now part of Uberon itself. The product was referred to in the 'quick_ac' rule (not called from anywhere, but which may be called manually by an user); in this instance we replace it by the $OWLSRC product.
* Remove old notes related to "step 2" of the main uberon.owl pipeline. * Remove TODO comments (should be replaced by tickets if they are still relevant). * Replace OWLTOOLS by ROBOT in the actual rule.
We remove a bunch of intermediate products on the way to build uberon.owl: * tmp/unreasoned.owl: only necessary as a dependency for the immediate next step of the pipeline, the only reason this file was written was because it was produced by OWLTOOLS; * tmp/materialized.owl, ext.owl, and tmp/merged.owl: all those products are actually identical, modulo some serialisation differences. We now produce directly uberon.owl in a single step from the $(OWLSRC) intermediate. All rules that depended on one of the deprecated intermediates are updated to depend on uberon.owl.
Contrary to what the comment is saying, we are _not_ temporarily injecting reflexivity axioms (maybe we should, but we have not done so since November 2021), so we remove that comment.
The `basic.owl` artifact is exactly the same thing as the `uberon-basic.owl` one, except for the different ontology IRI, so that entire rule is unnecessary.
We have a rule to produce uberon.yaml that is not used anywhere, and uberon.yaml is not an artifact we publish. So, remove it. Also add a link to the issue about the usefulness of the MAKEOBO/MAKEJSON custom rules.
The custom rule to create the BSPO mirror does the same thing as the ODK-generated rule, so the override is not needed.
The rule to create envo_import.owl had been overriden at some point because ENVO was "broken". Don't know if ENVO is still broken, but in any case we are no longer creating an envo_import.owl module since we have switched to the base merging mechanism, so the override is no longer needed.
We removed the custom rule that produced the 'ext.owl' artifact (which was identical to the standard 'uberon.owl'), so we also remove its declaration from the ODK configuration.
We amend the rule that creates `mirror/ro.owl` (and which overrides the ODK-generated rule) to: * avoid merging RO with BSPO at this point, because it is not needed (all mirrors will be merged together shortly after to create the merged import module anyway); * avoid re-downloading RO, which has already been downloaded by the standard ODK Makefile and written under $(TMPDIR)/mirror-ro.owl. We keep the injection of OBO shorthands with Owltools for now, unless a decision is made as to whether this is needed or not.
We used to hack the ZFA mirror to replace the use of "too specific" properties. This is not needed anymore as this has been fixed upstream.
Use ROBOT instead of Owltools to create the mirror/emapa.owl file. This is a simple merge of the hacked fixed-emapa.obo and the mmusdv.obo from the developmental stage repo, so there's nothing here that requires the use of Owltools.
We create a "fixed" version of ehdaa2 in tmp/fixed-ehdaa2.obo, but it is never used anywhere, so we can remove it.
Add more comments to the section about hacked mirrors, and re-format the recipes to make them more readable.
No logical change here. This commit simply re-organise the sections on mirrors and imports to have a clearer flow: * first all mirrors (both the standard, ODK-declared mirroring rules that we override, and the Uberon-specific custom mirrors); * then the rules to make the "local-" imports: first the generic rule, then the handful of ontology-specific rules. Comments are also re-written to match what is actually happening and to call things accurately (instead of seeding confusion by calling "mirrors" what are actually imports or the other way round).
All custom rules about `imports/*_import.owl` files are no longer relevant since the switch to the base merging mechanism. They can all be removed.
The custom Makefile contains several rules that create various unreleased files. They are currently scattered throughout the Makefile, so we regroup them into a dedicated section. In the process, we also: * add a comment explaining what those rules create; * reformat the rules for better readability; * replace OWLTOOLS by ROBOT wherever possible.
Rules that generate reports are scattered throughout the Makefile, so we regroup them in the REPORTS section and organise them in somewhat sensible subsections. Whenever possible, we add a brief comment to explain what the report is about. This does not concern all the reports about the bridges and the rest of the composite-* stuff; those rules probably deserve their own section.
When running the check-obo-for-standard-release script, disable the `xrf-abbs-check` that is dependent on a obsolete file that GO is seemingly no longer publishing. closes #2999
Similar to what we did previously for REPORTS. There are several rules related to the generation of bridges that are scattered at multiple places in the Makefile, we regroup them all in the BRIDGES section.
According to Chris Mungall, EMAP is dead. The bridge is currently imported into collected-mouse-with-stages.owl, which should probably be removed. For now, we just remove the rules to generate the EMAP bridge. The bridge file itself is kept as it is, so we can still build collected-mouse-with-stages.owl until we decide whether we can remove it as well or not.
This rule is a leftover of the time where Uberon was split between Uberon-ext and Uberon-core. This is no longer the case.
Similar to the recent changes, we regroup all rules that create subsets (and "views", which are merely subsets by another name) in a single section. Most rules are kept as they are, this commit is mostly just moving stuff around. In particular, most calls to Owltools are left untouched, since they are making use of features that are not available or not easily replicated with ROBOT.
Just moving rules around a bit for clarity. No changes to the rules themselves (yet).
There are currently 4 "flavours" of bridge checks, with several problems: 1. They all rely on Owltools. 2. The "quick" flavour, as implemented, does not do what the associated comments say it does. 3. There is actually no difference between the "standard" bridge check and the "full" bridge check. 4. The pre-requisites listed for each check do not necessarily correspond to the files that are actually needed. Here we fix all problems by re-defining only three flavours of checks, that differ by what is merged before running the reasoner: * quick check: uberon + external-disjoints + bridge to external ontology; * standard check: same as quick check + external ontology * extra full check: same as standard check + pending-disjoints. All three checks are implemented using ROBOT and with proper prerequisites. The top-level comment explaining the various flavours of checks is removed in favour of the comments located just above the actual implementation of the checks (this should help avoid another deviation between the implementation and the comment in the future). closes #3022
(Part 2 of fixing the bridge checks.) Now that only have three flavours of bridge checks, we amend the lists that dictates which bridge should be subjected to which tests. The lists are transformed into pre-requisites using Make's `foreach` construct instead of `patsubst`, whose use is not justified here.
The composite-metazoan-dv check does _not_ currently fail (which is not surprising since all disjointness axioms are stripped during the composite pipeline), so it does not need to be explicitly excluded.
Some views-generating rules were lost at the bottom of the Makefile, we bring them back to the SUBSETS & VIEWS section where they belong.
The tmp/allen-%.obo files are akin to the local imports -- they are used to generated the composite-* stuff. So move the rules generating those files to the corresponding section.
Move some checks performed on OBO files to the OBO checks subsection in the REPORTS section.
Wrap lines in the XML CATALOG section. Also amend the message to remove mention of `ro_import.owl`, which no longer exists since the switch to the base merging mechanism. Replace STRONG WARNING by WARNING. We are using STRONG WARNING so much that it does no longer mean anything.
Regroup all pre-requisites defining the various test suites (test, checks, and uberon-qc) in a single section near the top of the Makefile.
Merge the "Step 1" and "Step 2" sections into a single "BUILDING UBERON ITSELF" section.
Rename the "ROBOT PATTERNS AND TEMPLATES" section to "COMPONENTS", since there is only one rule in that section that deals with templates, but they all generate components. Move it before the RELEASE section.
The DOSDP section only contains rules that either do nothing, or that are dependent on files in a `modules` directory that do not exist in the repository. I suspect this is a remnant of the pre-ODK Makefile.
We need the `ssso-merged-uberon.obo` file from the developmental stage ontologies repo in order to build composite-metazoan (the file is imported in collected-metazoan), so we better explicitly add the update-stages step as a dependency. Otherwise we only get that file as a side-effect of the declaration of the emapa mirror.
The rule to generate tmp/external-disjoints.owl is not a "Nico hack", it is a prerequisite to all the bridge checks, so we move it to the BRIDGE CHECKS section. Of note, since the external disjointness axioms are maintained in a OBO file (from which the .owl file is derived with the rule we are talking about here), we could get rid of that rule and amend the bridge checks to use the OBO version directly.
The quick-qc test should be defined at the same place than the other test suites. Of note, as originally defined that test was dependent on the *preprocessed* source file but actually running on the normal -edit file.
Regroup some useful commands into a UTILITY COMMANDS section. Rename the "Nico hacks" section to PURGATORY, because it's probably unfair to put the blame for these hacks solely on Nico's shoulders.
The BASICRELS and TAXON_GCI_RELS variables are only used each in one section of the Makefile, so we move their definition to the section in which they are used. We remove the RELSIM variable which is not used anywhere.
Nico ongoing reviewIf the commit is checked, it means I have finished reviewing it (not that I agree with all of it).
|
matentzn
reviewed
Aug 19, 2023
matentzn
reviewed
Aug 19, 2023
matentzn
reviewed
Aug 21, 2023
matentzn
reviewed
Aug 22, 2023
matentzn
reviewed
Aug 23, 2023
matentzn
approved these changes
Aug 23, 2023
There was a problem hiding this comment.
Minor comment left, please resolve as you see fit.
Monumental work, looks fantastic. Just reviewing it took me a bit more than 3 hours, I cannot imagine how many dozens of hours it must have taking to make that PR!
Nico commit by commit review (just bookkeeping)
If the commit is checked, it means I have finished reviewing it (not that I agree with all of it).
- b01a116 (comments with real significance were left in, so all good)
- 3b49bbd (no questionable removals)
- 85192ee (no issues)
- b65fe5b (no issues)
- 2d76a28 (some minor issues, including reservations about removing ext.owl, and some further opportunities of removing noise from uberon.Makefile)
- e50dd88 (Damien made a corresponding issue, all good)
- e6e1b17 (no changes, just adding a FIXME)
- 4b462a5 (removing uberon.yaml, which has never been used)
- 87e5cdb (removing redundant uberon-base, all good)
- 0d13d64 (removing custom envo import; in any case, the brokenness was fixed a while back)
- 9cee90b (this is the removal of ext, which I initially opposed, but coming around to now, see Create redirect for the EXT.OWL release in the PURL config #3031)
- 7f143f1 (RO MIRROR special rule, has FIXME and associated ticket. All good).
- dd30ff3 (I didn't check that ZFA fixed all these relationships, but I trust Damien has checked it)
- 235ccb5 (minor source of possible risk, made comment, but no need to think of it).
- c6d75ea (its possible I made that mistake during the migration, and perhaps ehdaa2-local was supposed to depend on it, but, oh well).
- 435fe2e (good)
- 21fefc4 (fantastic)
- fd37d15 (removing import rules, not needed)
- 64da3e9 (non-functional reordering, these can probably be removed at some point)
- 094e00e (I spot checked the most important reports, (allcycles, gocheck), and it seems there are no functional changes. Only reordering).
- 123aa4c (mild concern, made a comment on makefile. Seems that GO file does exist?)
- ff6450c (this was a bigger commit, and I can't confidently say I have reviewed it line by line; but I reviewed the goals that I found most important, like
$(TMPDIR)/cl-core.oboand$(TMPDIR)/seed.obo). - 9630664 (emap overdue removal, ok)
- 62840c4 (redundant rule removal from ancient times, ok)
- 10b87bb (no functional changes, all good)
- 2d1747c (beautiful reorder)
- 775d824 (AWESOME! - I never really stuck long enough here to get how these checks were supposed to work)
- 2370209 (ok, learned something new, that I should not be using
patsubinmakefor simple for loops) - 3751a00 (clear rewriting of bfo check. nice)
- 3f6f476 (Cleanup)
- 0d18f9d (removing redundant rule, ok)
- bf30ae0 (nice simplification of developmental stage ontology use, no concerns)
- ea39a1d (simple cleanup, no concerns)
- c95e313 (reshuffling. some non functional changes, like removing uberon.owl from MBASE which is already covered by ext-weak. LGTM)
- e9fdef5 (small comment about redirection of import)
- 364f31a (no concerns)
- f1e3c06 (no concerns)
- 8bc44b9 (no concerns, not sure what this is and if it is effectual in any way)
- 327d198 (no concerns)
- 827b849 (no concerns)
- 6b335f4 (no concerns)
- d7bcf70 (great to see the test goals all grouped together. It would be nice to simplify this moving forward).
- 69c5a08 (no concerns, just comments)
- 9b5fc7f (made a separate issue Move the source of truth for HubMaps HRA subset to Uberon itself #3037)
- 0bba7b4 (brave, and correct decision IMO)
- 2cae55a (no concerns)
- 04e2c1e (no concerns)
- 6e5e3c5 (no concerns)
- 9e7c84c (I feel no offense that my hacks have been moved to PURGATORY)
- 7cee50e (no concerns)
- a719af2 (I don't like this but nothing related to PR)
This was referenced Oct 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR cleans up the custom
uberon.Makefileto, among other things:Overall, the goal is to make the Makefile reasonably readable.
This PR does not change any of the main products (e.g.,
uberon,uberon-base,composite-metazoan,subsets/human-view) in any meaningful way, beyond some serialisation-induced differences.Reviewers are strongly advised to review the PR commit by commit, rather than trying to review the PR-wide diff on
uberon.Makefile. Each commit has a commit message explaining what it does (and sometimes, why it does it).closes #2973