Skip to content

Conversation

@greschd
Copy link
Member

@greschd greschd commented Aug 21, 2024

Fixes #561

The EdgePropertyList retains the same list instance during its lifetime
for containing the edge property wrappers. This is done so that references
user code might hold to its items to correctly update its contents.

In contrast to other types, it means that the state is not re-fetched from
the _pb_object if it were to change outside of the EdgePropertyList's
control.
This led to a bug when constructing objects via _from_object_info, which
first default-constructs the object (in the process creating the EdgePropertyList)
and then modifies the _pb_object.

To fix this, the EdgePropertyList now stores whether its parent object was
stored when this list was last accessed. If it changes from unstored to stored,
the following logic is applied:

  • If the current list is already populated (the "regular" case), only a sanity check
    for matching length is performed
  • If the current list is empty, it is re-fetched from the parent object. This is the
    case which occurs during construction with _from_object_info

Since the edge property list can be in an inconsistent state (empty when it shouldn't
be) while the parent is unstored, we disallow accessing it in this state. It is still
allowed however to fully replace the contents.

This PR also fixes a bug in tree_object_from_resource_path, where the channel
argument was still used instead of server_wrapper.

@greschd greschd linked an issue Aug 21, 2024 that may be closed by this pull request
@greschd
Copy link
Member Author

greschd commented Aug 21, 2024

@roosre this is a fairly significant bugfix, I'd suggest we do a 0.1b2 release for it (and only drop Python 3.9 support after that).

@greschd greschd mentioned this pull request Aug 21, 2024
@codecov
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.91%. Comparing base (eb5056c) to head (661edaf).
Report is 2 commits behind head on main.

Files Patch % Lines
...ore/_tree_objects/_grpc_helpers/property_helper.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
+ Coverage   91.93%   92.91%   +0.97%     
==========================================
  Files          82       82              
  Lines        4242     4291      +49     
==========================================
+ Hits         3900     3987      +87     
+ Misses        342      304      -38     

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

@greschd greschd force-pushed the fix/561-edgepropertylist-is-empty-when-loading-an-existing-model branch from 40f457c to c9a0a48 Compare August 21, 2024 15:46
@greschd greschd force-pushed the fix/561-edgepropertylist-is-empty-when-loading-an-existing-model branch from 4d7db86 to b6bd031 Compare August 22, 2024 06:21
@greschd greschd marked this pull request as draft August 22, 2024 07:34
@greschd

This comment was marked as outdated.

@greschd greschd marked this pull request as ready for review August 22, 2024 08:09
@greschd greschd merged commit 31a3798 into main Aug 23, 2024
@greschd greschd deleted the fix/561-edgepropertylist-is-empty-when-loading-an-existing-model branch August 23, 2024 22: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.

EdgePropertyList is empty when loading an existing model

3 participants