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

Download component from source url workflow can't find file to copy #1060

Open
anitacaron opened this issue May 17, 2024 · 7 comments
Open
Assignees
Labels
Milestone

Comments

@anitacaron
Copy link
Contributor

When the component is different, it copies from the tmp folder, but when it's downloading for the first time, it shouldn't try to copy.

Error on test PR https://github.com/obophenotype/uberon/actions/runs/8389532879/job/22975944727

Screenshot 2024-05-17 at 11 36 07

@anitacaron anitacaron self-assigned this May 17, 2024
@anitacaron
Copy link
Contributor Author

I found out that the issue is in the QC. Now, we need to set MIR=false, and to download the component, the condition is MIR=true.

component-download-{{ component.filename }}: | $(TMPDIR)
if [ $(MIR) = true ] && [ $(COMP) = true ]; then $(ROBOT) merge -I {{ component.source }} \{%- if component.make_base %}
remove {% if component.base_iris is not none %}{% for iri in component.base_iris %}--base-iri {{iri}} {% endfor %}{% else %}--base-iri $(OBOBASE)/{{ project.id.upper() }} {% endif %}--axioms external --preserve-structure false --trim false \{% endif %}
annotate --ontology-iri $(ONTBASE)/$@ $(ANNOTATE_ONTOLOGY_VERSION) -o $(TMPDIR)/[email protected]; fi

What are your thoughts? @matentzn @gouttegd

@gouttegd
Copy link
Contributor

gouttegd commented May 17, 2024

I think that deep down, it’s all a consequence of the old approach of using shell conditionals instead of Make conditionals (see #912). Make is unaware of shell conditionals, so when you have a rule like this:

component-download-{{ component.filename }}: | $(TMPDIR) 
 	if [ $(MIR) = true ] && [ $(COMP) = true ]; then ...ALL THE CODE OF THE RULE... ; fi

where the entirety of what the rule does is within a shell conditional, when the condition is false Make will have no idea that the rule didn’t actually do anything. From Make’s point of view, the shell code has been executed, that’s all that matters.

We should convert all those shell conditionals to Make conditionals as we recently did for the mirroring rules (#973).

Something like that:

ifeq ($(COMP),true)
# COMP=true -> we regenerate the components

ifeq ($(MIR),true)
# MIR=true -> even the components that are mirrored from a foreign source

.PHONY: component-download-hra_subset.owl
component-download-hra_subset.owl: | $(TMPDIR)
	$(ROBOT) merge -I https://raw.githubusercontent.com/hubmapconsortium/ccf-validation-tools/master/owl/UB_ASCTB_subset.owl \
	annotate --ontology-iri $(ONTBASE)/$@ $(ANNOTATE_ONTOLOGY_VERSION) -o $(TMPDIR)/$@.owl

$(COMPONENTSDIR)/hra_subset.owl: component-download-hra_subset.owl
	if cmp -s $(TMPDIR)/component-download-hra_subset.owl.owl $(TMPDIR)/component-download-hra_subset.owl.tmp.owl ; then echo "Component identical."; \
      else echo "Component is different, updating." &&\
		cp $(TMPDIR)/component-download-hra_subset.owl.owl $(TMPDIR)/component-download-hra_subset.owl.tmp.owl &&\
		$(ROBOT) annotate -i $(TMPDIR)/component-download-hra_subset.owl.owl --ontology-iri $(ONTBASE)/$@ $(ANNOTATE_ONTOLOGY_VERSION) -o $@; fi

.PRECIOUS: $(COMPONENTSDIR)/hra_subset.owl

endif

# Here, we can re-generate the other components (those that don't depend
# on a foreign source, and therefore that can be built even under MIR=false)

else

# COMP=false -> Not regenerating any component; if any rule depends on a
# component, Make would simply notice that the component files already exist
# in `$(COMPONENTSDIR)` (assuming the components are committed to the
# repository, which they normally should be), and will not do anything.

endif

@anitacaron
Copy link
Contributor Author

Yeah, but still, if MIR=false, it won't download the component, right?

@gouttegd
Copy link
Contributor

No indeed, but isn’t that what is wanted here? We only want to refresh that component when MIR=true.

@anitacaron
Copy link
Contributor Author

Actually, the second goal only checks for COMP=true and not the MIR. I agree that using the make condition would be clearer.

$(COMPONENTSDIR)/{{ component.filename }}: component-download-{{ component.filename }}
if [ $(COMP) = true ]; then if cmp -s $(TMPDIR)/component-download-{{ component.filename }}.owl $(TMPDIR)/component-download-{{ component.filename }}.tmp.owl ; then echo "Component identical."; \
else echo "Component is different, updating." &&\
cp $(TMPDIR)/component-download-{{ component.filename }}.owl $(TMPDIR)/component-download-{{ component.filename }}.tmp.owl &&\
$(ROBOT) annotate -i $(TMPDIR)/component-download-{{ component.filename }}.owl --ontology-iri $(ONTBASE)/$@ $(ANNOTATE_ONTOLOGY_VERSION) -o $@; fi; fi

@anitacaron
Copy link
Contributor Author

I'll create the PR, thanks.

@gouttegd
Copy link
Contributor

Actually, the second goal only checks for COMP=true and not the MIR

Yeah, but since the rule that this one depends on does also check MIR, this does not make any sense. In effect, and unless we happen to have a $(TMPDIR)/component-download-{{ component.filename }}.owl file that would be a leftover from a previous run (the kind of things we should definitely not rely upon), we can only regenerate that component when both MIR and COMP are true. I assume this is the desired behaviour.

@anitacaron anitacaron added this to the 1.6 milestone May 31, 2024
@anitacaron anitacaron added the bug label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants