Reinstate sync pipeline #403
Conversation
Previously, the pipeline did not fail when `UST_BACKTRACE=full semsql make $*.db -P config/prefixes.csv` failed
| @@ -422,6 +426,9 @@ $(MAPPINGSDIR)/rejected-mappings.tsv: | |||
| $(MAPPINGSDIR)/rejected-mappings-sssom.tsv: $(MAPPINGSDIR)/rejected-mappings.tsv | |||
| sssom parse $< -m metadata/mondo.sssom.config.yml --no-strict-clean-prefixes -o $@ | |||
|
|
|||
| tmp/mondo-ingest.owl: mondo-ingest.owl | |||
There was a problem hiding this comment.
Suggestion -> tmp/mondo-ingest.db: mondo-ingest.db
Details
Changes
(1) Change the prereq here to mondo-ingest.db
$(REPORTDIR)/%.subclass.added.robot.tsv $(REPORTDIR)/%.subclass.confirmed.robot.tsv $(REPORTDIR)/%.subclass.direct-in-mondo-only.tsv $(REPORTDIR)/%.subclass.added-obsolete.robot.tsv: tmp/mondo-ingest.db tmp/mondo.db tmp/mondo.sssom.tsv(2) For the same target as above, remove $(TMPDIR) from this line
--mondo-ingest-db-path $(TMPDIR)/mondo-ingest.db(2) Delete goal for tmp/mondo-ingest.owl
Rationale
We already have a mondo-ingest.owl. We can just make a mondo-ingest.db. There is not need I think for a duplicate file in tmp/. Also, the .gitignore is already set up for *.db so no worry about it getting committed.
I felt like we had already made this change months ago, but I think what happened was we had a similar situation for mergd.owl/.db and tmp/merged.owl/.db. We decided to stick with only 1 copy of those files (the tmp/ version).
There was a problem hiding this comment.
I like as few files as possible in the src/ontology dir -> hence that suggestion..
There was a problem hiding this comment.
Suggestion -> mondo-ingest.owl/.db -> tmp/
How about this, then? mondo-ingest.owl is actually in the .gitignore. We could just move it and its .db into tmp/.
There was a problem hiding this comment.
I agree with Nico's addition here.
There was a problem hiding this comment.
mondo-ingest.owl is in "gitignore", but it is used by "make deploy-release". We can reconsider this when we decide that we want to publish a db version of Mondo ingest for easier access in OAK by other parties.
There was a problem hiding this comment.
@matentzn
(1) I'm not talking about publishing a .db version. I'm just think we have needless duplication of goals / files here.
(2) By deploy-release (I don't see it) do you mean deploy-mondo-ingest? It's NP to me that it's in the .gitignore but also in a release. Usually that's the case for release artefacts.
(3) There are no prereqs for deploy-mondo-ingest. Not a huge deal but I suppose all the release artefacts should be listed.
There was a problem hiding this comment.
Yes, we decided to decouple deploy commands from everything as they are dependent on files after the are moved to the release directory. I can explain in a call how this works if you like details!
There was a problem hiding this comment.
That answers (3). I can just take your word for it.
That doesn't answer my questions (1) and (2) above. But we have bigger fish to fry if you don't wanna worry about it.
There was a problem hiding this comment.
(2) deploy-mondo-ingest -> Yes, that is what I meant. It picks the release files from the release directory and uploads them to github (just did it yesterday in #417)
(1) Its a value judgement. I kinda dont like (at all), that ODK puts anything (any intermediate file) in src/ontology. That was a design mistake. It makes that folder, which has key confguration, feel cramped an unwieldy. I would prefer all files, or at least as much as possible, to be generated in tmp/ to be less cramped. I am not taking a super hard stance here though and can be convinced otherwise.
There was a problem hiding this comment.
Thanks for providing me those answers!
I didn't realize ODK was doing src/ontology/$(ONT).owl but I see that now and that makes sense.
Co-authored-by: Joe Flack <joeflack4@gmail.com>
| @@ -422,6 +426,9 @@ $(MAPPINGSDIR)/rejected-mappings.tsv: | |||
| $(MAPPINGSDIR)/rejected-mappings-sssom.tsv: $(MAPPINGSDIR)/rejected-mappings.tsv | |||
| sssom parse $< -m metadata/mondo.sssom.config.yml --no-strict-clean-prefixes -o $@ | |||
|
|
|||
| tmp/mondo-ingest.owl: mondo-ingest.owl | |||
There was a problem hiding this comment.
I agree with Nico's addition here.
@joeflack4 @twhetzel
This PR fixes the sync pipeline - in the end, the error was small, but it took quite some needle in the haystack searching..
I would suggest to review this, merge it into #397 (this is where this PR is directed towards), and then merge #397 as a whole.
Once its done, I will rerun everything again.