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

Refreshing of mirrors should not be blocked by IMP=false #946

Closed
gouttegd opened this issue Oct 25, 2023 · 1 comment · Fixed by #973
Closed

Refreshing of mirrors should not be blocked by IMP=false #946

gouttegd opened this issue Oct 25, 2023 · 1 comment · Fixed by #973
Assignees
Labels
Milestone

Comments

@gouttegd
Copy link
Contributor

Currently, the generated rule that refreshes a mirror only does something when both MIR and IMP are true:

mirror-cl:
    if [ $(MIR) = true ] && [ $(IMP) = true ]; then ... ; fi

This makes it impossible to have a custom workflow that depends on a mirror and that could be run under IMP=false. Such a workflow would have to be run under IMP=true in order to be sure the required mirror has indeed been downloaded and is available locally.

Requiring both MIR and IMP to be true for a mirror to be refreshed only makes sense if we assume that the only purpose of a mirror is to be used to create an import module. That may be true in many (possibly most) cases, but there are definitely situations where one might want to use a locally mirrored foreign ontology for other things in addition to creating an import module. Uberon for example is re-using the locally mirrored CL and ZFA ontologies as part of the bridge generation pipeline, which has nothing to do with the imports pipeline.

The only benefit of the current approach that I can see is that it allows user to just have to specify IMP=false for completely bypassing both the imports pipeline and the mirroring pipeline. But the price for that is what is to me an inconsistent behaviour: if I run a workflow with MIR=true IMP=false, I would expect the mirrors to be refreshed (MIR=true) and the import modules not being re-generated (IMP=false).

I believe the rules that refresh the mirror should strictly depend on MIR only.

@gouttegd gouttegd added the bug label Oct 25, 2023
@matentzn
Copy link
Contributor

Requiring both MIR and IMP to be true for a mirror to be refreshed only makes sense if we assume that the only purpose of a mirror is to be used to create an import module.

This is indeed the historical reason. I agree with your proposal! Lets have mirrors only depend on MIR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants