feat: added support for the libraries linking and referencing from model#619
feat: added support for the libraries linking and referencing from model#619wingedfox wants to merge 27 commits into
Conversation
…s of the models. bugfix: wrap FIltering into Optional accessor to let models without filtering (which is normal) be opened using .model_root, last release fails here
… due to wrong match to basic ModelElement, added specific match for re:RecCatalog
…: , New accessor:
…tes, not the lists, must be a single relation
… a single relation
Wuestengecko
left a comment
There was a problem hiding this comment.
I'm a bit confused on who this API is for and when/how it should be used:
- Is it for linking in an entirely new library? In the docstring example,
project.extensionsis already pre-filled, which is confusing in that case. - Is it for repairing a broken model? Then it'd be good to have some documentation on the symptoms of brokenness (also how it shows up in Capella, not just py-capellambse), and possibly create a standalone script that can directly be called (as I mentioned in a comment).
- Is it meant to be called by some internal APIs/functions that don't exist yet / haven't been adjusted to call it yet?
Yes, it targets use of entirely new libraries without running Capella to link library and operate REC/RPL in the model. This PR does not target model fixing, but adds new function of
It seems nobody needed this before that's why relatively simple PR introduced many changes. Basic scenario is
|
…ct after initialization
…cation on empty model can be avoided
|
Here's the reproduced bug related to broken Filtering Extension and why Optional was needed to hide this error |
|
Looks like the last merge has gone wrong a bit and a lot of commits are now duplicated. Could you please run these commands locally, which should fix it? (Make sure to commit or stash first, so that git switch -d ba9639869
git pull --no-ff https://github.com/dbinfrago/py-capellambse master
git checkout --theirs src/capellambse/metamodel/pa/__init__.py
git add src/capellambse/metamodel/pa/__init__.py
git merge --continue
git rebase 29b9cee8f master --onto @
git switch master |
Wuestengecko
left a comment
There was a problem hiding this comment.
I'll also do some more thorough testing in the next days, but in general it looks mostly good - is the PR ready from your perspective, or do you have more to implement?
Corrected to ownedFunctions, not ownedPhysicalFunctions and PhysicalFunction, not PhysicalComponent
|
I've been working on the very deep model modifications and I'm not sure what I can find and fix. For instance, I've found that in some cases asking collection to add IntegerPropertyValue I get String instead, in other cases - Boolean. I miss methods to "copy" elements and element creation for non-collection elements like ownedMinCard/ownedMaxCard. Hope we can merge this PR before anything else come, I have reached 95% MVP readiness with current PR. What I've been missing a lot is the new ELK routing, because my models have something around 11K elements per model with around 20 Components/100 functions/500 exchanges per layer. |
Removed line adds extra empty string into the values which causes zip fail
|
here's the article and tool based on py-capellambse |
|
I've been waiting for a resolution of this PR... |
|
@Wuestengecko @ewuerger could you please take a look? |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Wuestengecko
left a comment
There was a problem hiding this comment.
Hi! Sorry for coming back to you so late.
I do have a few more comments. Especially the duplicated tree part looks concerning, we should definitely take a closer look at what's causing that. I think it'd be great to have a few additional test cases there (and for the library code more generally; we don't really have a lot of tests for that part at all yet).
| # they do not pass "is" check but have the same IDs | ||
| for fragment, tree in self.trees.items(): | ||
| if tree.root.attrib.get("id") == root.attrib.get("id"): | ||
| return (fragment, tree) |
There was a problem hiding this comment.
This sounds odd. There should only ever be exactly one LXML entity per model element, especially the project root (under which everything else is nested).
-
Does this refer to the root of the (newly linked) library project? In this case, we should find it in
self.trees- if not, we're missing the entry here. -
If it actually is the same model, we have accidentally duplicated the project tree somehow (though it's not obvious to me how that would happen right now), and are now mixing elements from both tree instances.
-
Could this be due to some buggy interaction (or perhaps race) with the namespace map update code? (Be aware that capellambse currently isn't thread-safe, so this might be caused by multi-threaded downstream code.)
All of these could easily lead to corrupted models and data loss, so this needs to be addressed before merging. If you can, could you add a test case which hits this case?
| ``` | ||
| """ | ||
| handler = self.resources[str(lib)] | ||
| _h, filename = _derive_entrypoint(handler) |
There was a problem hiding this comment.
| _h, filename = _derive_entrypoint(handler) | |
| _, filename = _derive_entrypoint(handler) |
|
|
||
| lib = model.project.extensions[0].reference.library | ||
| ``` | ||
| """ |
There was a problem hiding this comment.
The docs build is failing because sphinx doesn't like this docstring in particular.
-
There's no "Description" heading. The full description goes right after the one-line summary, separated by a single blank line and no extra headings in between.
-
Can you move the code example into an "Examples" section after "Parameters"? I think this is where it'd fit best in this case.
-
Could you please also add a test case (or multiple) which exercises this new code? This would also serve nicely as secondary documentation. We already have a few test models in the
conftest.Modelsdict which should come in handy here.
You can test the docs build locally as well (which should be much faster than waiting for CI) with a simple make docs.
| from __future__ import annotations | ||
|
|
||
| import capellambse.model as m | ||
| from capellambse.metamodel import re |
There was a problem hiding this comment.
I don't know why Github isn't running CI for this PR. Could you please make sure you run all the pre-commit hooks locally with pre-commit run -a ? Here we have an unused import which the hooks would warn about (and automatically remove for you).
| sending_end = m.Single( | ||
| m.Association["MessageEnd"]((NS, "MessageEnd"), "sendingEnd") | ||
| ) | ||
| receiving_end = m.Single( | ||
| m.Association["MessageEnd"]((NS, "MessageEnd"), "receivingEnd") | ||
| ) |
There was a problem hiding this comment.
Please move the bracketed type annotation up a level to the m.Single instance instead of whatever it wraps. This helps keep things nice and consistent (i.e. it's always the outermost descriptor that has the annotation).
-m.Single(
- m.Association["Annotation"](...)
+m.Single["Annotation"](
+ m.Assocation(...)
)Same also in the other metamodel files you modified.
| if not next( | ||
| filter( | ||
| lambda el: re.search(str(ref_name), el.attrib["href"]), | ||
| meta_self.iterchildren("additionalResources"), | ||
| ), | ||
| None, | ||
| ): |
There was a problem hiding this comment.
We're doing a simple substring match here, no need to use a regex. (Same applies a few lines further down for .capella files.)
Also, the next(filter(lambda el: ..., ...), None) construct can be rewritten using any() and a generator expression, which saves a little bit of overhead.
| if not next( | |
| filter( | |
| lambda el: re.search(str(ref_name), el.attrib["href"]), | |
| meta_self.iterchildren("additionalResources"), | |
| ), | |
| None, | |
| ): | |
| if not any( | |
| str(ref_name) in el.attrib["href"] | |
| for el in meta_self.iterchildren("additionalResources") | |
| ): |
| last = next( | ||
| filter( | ||
| lambda el: re.search(str(ref_name), el.text), | ||
| XP_SEMANTIC_RESOURCES(aird_self.root), | ||
| ), | ||
| None, | ||
| ) |
There was a problem hiding this comment.
The same points as above also apply here (regex for substring match and filter+lambda overhead). Also, here we're doing the XPath lookup twice, which I think we could combine into a single iteration like this (notice that the else belongs to the loop itself, not the enclosed if):
for el in XP_...:
if str(ref_name) in el.text:
break
else:
sr = el.makeelement(...) # etc.| elems = list(itertools.islice(it, idx.start, idx.stop, idx.step)) | ||
| values = [i.text for i in elems] | ||
| values[idx] = value | ||
| for e, v in zip(elems, values, strict=True): | ||
| e.text = v |
There was a problem hiding this comment.
Good catch, this is/was indeed bugged. But I think the fix is incomplete: Now the for loop is just setting the current values instead of the new ones. Instead, we can simply iterate over the islice directly, i.e.:
elems = itertools.islice(it, idx.start, idx.stop, idx.step)
for e, v in zip(elems, value, strict=True):
e.text = v| if metadata is None: | ||
| raise RuntimeError("Cannot find <Metadata> in primary .afm file") |
There was a problem hiding this comment.
This exception message doesn't quite fit anymore, since this function is also used for auxiliary .afm files. Let's simply show the filename instead:
| if metadata is None: | |
| raise RuntimeError("Cannot find <Metadata> in primary .afm file") | |
| if metadata is None: | |
| raise RuntimeError("Cannot find <Metadata> in {afm.filename}") |
Added method into the loader to support libraries linking.
Use of the resources and referencing them in the extensions does not work. lookup fails with missing fragment.
Here I fix this by allowing explicit library loading and linking into a model.