Skip to content

Conversation

@silverweed
Copy link
Contributor

This Pull request:

adds a virtual Close method to RPageStorage, to be used by pythonizations of RNTupleReader and RNTupleWriter.
Also updates those pythonizations to use it.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! Two remaining comments from my side:

  • It would be nice to add also a simple test for the reader and writer
  • What happens if I call reader.Close() and then somehow try to access it again? From the changes it's not immediately clear to understand if I get an error/info or there could be unexpected behaviour.

@github-actions
Copy link

Test Results

    22 files      22 suites   3d 11h 43m 37s ⏱️
 3 770 tests  3 769 ✅ 0 💤 1 ❌
74 989 runs  74 988 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 115bcb0.

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM.

@silverweed
Copy link
Contributor Author

Rethinking about this, I now think this is not the best solution.

As far as I see it, the Close method is not really needed on the C++ side and it's just a side effect of Python needing a "valid but invalid" object after the context manager exits. If we go and add this to the C++ side we will end up with additional unwarranted code complexity stemming from the fact that the Reader and Writer lose the invariant that the source/sink is valid as long as they are valid, which will result in adding validity checks around which would not be needed in principle.

I think the proper solution would be to do this purely on the Python side: in the __exit__ method we reset the internal pointer to the reader/writer (assuming this is possible) and hijack the proxy object to raise an exception whenever you try to access a method of the underlying object.

@vepadulano @jblomer opinions on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants