Skip to content

Conversation

@greschd
Copy link
Member

@greschd greschd commented Aug 22, 2024

Implement a recursive_copy helper function, which:

  • takes a list of source objects
  • walks its children and (optionally) linked objects
  • copies the sub-tree to a new location, specified by
    a dict mapping old parent to new parent [1]

Adds a dependency on networkx, for computing the
order in which objects should be stored s.t. all their
dependencies (parent, linked objects) are already stored.
We may reuse the dependency graph produced for this
task in other contexts.

Note that the copy operation may fail if a linked object is present
in the API layer, but not supported by PyACP yet. In this case,
the linked object is still detected (since this is done by iterating through
the protobuf object), but the object cannot be instantiated.
I think this is acceptable, as it's a temporary problem and may be more
desirable than silently losing the link.

Other changes:

  • added a .clone() method to the edge property lists. This was
    used in the initial implementation of this features. Since it's
    consistent with the API of other objects, the method is kept.
    Each edge property list type separately implements .clone(). [2]
  • add a .parent property to tree objects, which instantiates / gets
    the parent via its resource path
  • make tree objects hashable
  • upload the XML coverage report separately for all unittest runs,
    and distinguish them (by Python version and server version) using
    flags.

[1] more than one new parent may be needed, for example
when copying a Modeling Ply (parent: Modeling Group) which
links to a Fabric (parent: Model).
[2] in general, the GenericEdgePropertyType classes have a
lot of code duplication; this can be dealt with in a later PR (not
urgent).

@greschd greschd force-pushed the feat/recursive_clone branch from a6186f0 to a300d54 Compare August 22, 2024 21:05
@greschd greschd changed the base branch from main to fix/561-edgepropertylist-is-empty-when-loading-an-existing-model August 22, 2024 21:05
@codecov
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 97.88360% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.31%. Comparing base (35d0a69) to head (27174ef).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../_tree_objects/_grpc_helpers/edge_property_list.py 75.00% 1 Missing ⚠️
...ore/_tree_objects/_grpc_helpers/property_helper.py 75.00% 1 Missing ⚠️
src/ansys/acp/core/_tree_objects/base.py 93.75% 1 Missing ⚠️
src/ansys/acp/core/_typing_helper.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
+ Coverage   93.06%   93.31%   +0.25%     
==========================================
  Files          82       84       +2     
  Lines        4297     4445     +148     
==========================================
+ Hits         3999     4148     +149     
+ Misses        298      297       -1     
Flag Coverage Δ
python-3.10 93.18% <97.88%> (?)
python-3.11 93.22% <95.76%> (?)
python-3.12 93.22% <95.76%> (?)
server-242 93.00% <95.76%> (?)
server-latest 93.31% <97.88%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@greschd greschd force-pushed the feat/recursive_clone branch from eeb6c05 to c2a986b Compare August 23, 2024 12:42
Base automatically changed from fix/561-edgepropertylist-is-empty-when-loading-an-existing-model to main August 23, 2024 22:32
@greschd greschd force-pushed the feat/recursive_clone branch from 151a17e to e6e443b Compare August 26, 2024 06:12
@greschd greschd marked this pull request as ready for review August 26, 2024 07:03
Copy link
Contributor

@roosre roosre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

Please look at my questions and comments.

FYI: A reverse dependency graph lookup would be very helpful as well. For instance where is Fabric XY used? Or with which objects is OSS XZ linked? But that can be addressed in a separate PR.

@greschd greschd force-pushed the feat/recursive_clone branch from ccec699 to 435df2a Compare September 17, 2024 14:46
@greschd greschd changed the base branch from main to maint/drop_python_39 September 17, 2024 14:47
@greschd greschd force-pushed the feat/recursive_clone branch from 0597b6d to 82f0ed9 Compare September 18, 2024 08:07
@greschd greschd force-pushed the feat/recursive_clone branch from 9c9e0ff to 5525c3d Compare September 18, 2024 12:56
@greschd
Copy link
Member Author

greschd commented Sep 18, 2024

@roosre I have implemented the discussed changes to the interface of the recursive_copy function.

On the implementation side, the main change was using the _pb_object.properties to find linked objects instead of going through the _GRPC_PROPERTIES. This has the advantage that it doesn't always require instantiating the object.

Base automatically changed from maint/drop_python_39 to main September 19, 2024 07:17
@greschd greschd force-pushed the feat/recursive_clone branch from 60e2bec to 2222235 Compare September 19, 2024 09:01
@greschd greschd force-pushed the feat/recursive_clone branch 2 times, most recently from 6deb448 to 211b8ba Compare September 19, 2024 11:12
@greschd greschd force-pushed the feat/recursive_clone branch from 211b8ba to 9a3d478 Compare September 19, 2024 11:15
@greschd greschd enabled auto-merge (squash) September 19, 2024 11:23
@greschd greschd merged commit 7e20b60 into main Sep 19, 2024
@greschd greschd deleted the feat/recursive_clone branch September 19, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants