Allow excluding an import from the merged module - #23
Open
gouttegd wants to merge 6 commits into
Open
Conversation
Sometimes you might want to merge _almost_ all import modules, _except_
a handful of them that you would like to keep as individual import
modules instead -- for example, if one particular import requires some
custom extraction parameters.
This commit lays the groundwork for that, by allowing to declare that an
import module must be excluded from the merged module:
```yaml
import_group:
use_base_merging: true
products:
# alice and bob are merged into merged_import,
# but charlie is imported separately as charlie_import
- id: alice
- id: bob
- id: charlie
exclude_from_merge: true
```
In principle, it would have been possible to achieve the same effect
without having to add an explicit `exclude_from_merge` setting: if an
import module has its own extraction settings (e.g. its own
`module_type`) that differs from the group-level defaults (whether they
are implicit or explicit), this could be enough to infer that this
module must be kept separate from the merged module. But doing so would
introduce a risk of breaking existing (erroneous) configurations.
A somewhat more elegant design would have been to do something similar
to what we do for SSSOM mapping sets, and have `merged` as a new type of
import modules. This would allowed to do something like:
```yaml
import_group:
products:
- id: alice
- id: bob
- id: charlie
- id: merged
module_type: merged
source_modules:
- alice
- bob
```
which would in turn have allowed to have _several_ merged import
modules, each possibly with its own extraction settings.
But that would have been too much of a change. The `use_base_merging`
setting and the idea of a single `merged_import` module have been in
place for several years already, it's likely too late to change that.
Refactor the templates for * the catalog file, * the -edit file, * the import module placeholder files, to make them take into account the fact, even with `use_base_merging`, there may still be individual import modules. This incidentally makes the concerned templates somewhat simpler, because they don't even need to test on `use_base_merging` anymore: all they have to do is to iterate on the pre-computed `project.import_group.import_names` list, which is the ready-to-use list of all import modules that needs to be imported (when base merging is not used, this is the list of all import modules; when base merging _is_ used, this is `merged` + any explicitly excluded import module).
The last step to support import modules that are excluded from the
merged module is to make sure the standard Makefile can handle them.
Since the model automatically puts any excluded module in the
`project.import_group.special_products` list, mostly all we need is to
move the code that iterates over that list _outside_ of the
`non-use-base-merging` conditional.
That is, where we used to have:
if project.import_group.use_base_merging
... rule to produce the merged module ...
else
... default rules ...
for imp in project.import_group.special_products
... special rule for imp ...
endfor
endif
we are now doing instead:
if project.import_group.use_base_merging
... rule to produce the merged module ...
else
... default rules ...
endif
for imp in project.import_group.special_products
... special rule for imp ...
endfor
matentzn
reviewed
Sep 5, 2026
| for product in self.project.import_group.products: | ||
| cmd += f" --add {base}/imports/{product.id}_import.owl" | ||
| for name in self.project.import_group.import_names: | ||
| cmd += f" -- add {base}/imports/{name}_import.owl" |
Contributor
There was a problem hiding this comment.
Suggested change
| cmd += f" -- add {base}/imports/{name}_import.owl" | |
| cmd += f" --add {base}/imports/{name}_import.owl" |
| refresh-imports-excluding-large: | ||
| $(MAKE) IMP=true MIR=true PAT=false IMP_LARGE=false clean all_imports | ||
|
|
||
| {% if not project.import_group.use_base_merging or project.import_group.special_products -%} |
Contributor
There was a problem hiding this comment.
what about refresh-merged? will that still exist when you DONT use exclusions?
Collaborator
Author
There was a problem hiding this comment.
No, but I don’t consider that important. If there are no excluded modules, then you can just use refresh-imports to refresh the merged module.
The point of the refresh-%, when base merging is used, is to be able to refresh an excluded module individually without having to refresh everything. Without any excluded module, it gives you nothing than refresh-imports doesn’t already.
| actually equivalent to setting ``make_base`` to true. | ||
| """ | ||
|
|
||
| exclude_from_merge: bool = False |
Contributor
There was a problem hiding this comment.
Nonsense, optional comment: if every product is exclude_from_merge: true, you get ALL_MIRRORS = empty, $(MIRRORDIR)/merged.owl with no prerequisites, and a robot merge with no --input — while merged_import is still in IMPORT_ROOTS and still imported by the edit file.
Collaborator
Author
There was a problem hiding this comment.
I would consider that a bogus setup (one that would probably deserve at least a warning), but we can protect against that.
Aka your weekly remember not to drink and code.
If base merging is enable, explicitly check that we do have something to merge in the merged module (i.e. not all modules are explicitly excluded), or disable base merging altogether.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 makes it possible to explicitly exclude an import module from the merged module, when
use_base_merging. That is, it allows to do the following:This is mostly done by having
src/incatools/odk/model.pypre-compute a handful of lists: the list of all the modules that should be merged, the list of all the modules that should not be merged, and the list of all the names of the final, real import modules (in the example above, this would be[merged charlie]). Then, the templates are updated to use whatever list is relevant for them. For example, to generate theImport()declarations in the-editfile, what matters is the list of all real import modules; to generate themerged_mirror.owlfile, what we need is the list of the merged modules, etc.To produce the Makefile rules needed for the excluded import modules, we reuse the existing logic initially created for the “special products” (all import modules that require a custom rule for one reason or another): excluded import modules are simply added to that list.
closes INCATools/ontology-development-kit#1189