Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow not calling finish() when using the writers #442

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

jmcarcell
Copy link
Member

Forgetting finish() is likely a mistake and by the time the writer is destroyed what the user probably wanted to is to write. The RNTuple ROOT writer works like that and writes when the object is destroyed. For the RNTuple writer in podio this is already done.

BEGINRELEASENOTES

  • Allow not calling finish() when using the writers

ENDRELEASENOTES

@tmadlener
Copy link
Collaborator

Very nice, and the right thing to do, IMHO. The only potential issue I see is anything that throws in the destructor will cause the program to terminate, so we would maybe have to try-catch in there?

Taking this a bit further: Do we ever need the functionality to "finish early"? I.e. long before a writer is destructed? If not we could just introduce a private function that does what finish does now, call that in the destructor and make the current finish a no-op and deprecate it.

@jmcarcell
Copy link
Member Author

Very nice, and the right thing to do, IMHO. The only potential issue I see is anything that throws in the destructor will cause the program to terminate, so we would maybe have to try-catch in there?
But that is the same behavior we have right now, right? Probably no one was wrapping the finish() inside a try-catch. I can add one

Taking this a bit further: Do we ever need the functionality to "finish early"? I.e. long before a writer is destructed? If not we could just introduce a private function that does what finish does now, call that in the destructor and make the current finish a no-op and deprecate it.

This I don't know, if someone wants it. I guess you hold everything in memory until you write so there could be a case where it's better to write earlier. I don't think having the flexibility hurts but surely most people will stop using finish() if not's necessary so deprecating it also wouldn't be bad.

@hegner
Copy link
Collaborator

hegner commented Jul 11, 2023

I do not think we need an explicit finish here any more. What we could keep in mind is an explicit option to flush buffers to disk so that things can be written out.
I however do not see yet a reasonable request for it.

@hegner hegner merged commit 9ac7d32 into AIDASoft:master Jul 11, 2023
17 of 18 checks passed
hegner pushed a commit to hegner/podio that referenced this pull request Jul 27, 2023
Co-authored-by: jmcarcell <jmcarcell@users.noreply.github.com>
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