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

Add numpy-style docstrings #12

Merged
merged 9 commits into from
Jan 21, 2024
Merged

Add numpy-style docstrings #12

merged 9 commits into from
Jan 21, 2024

Conversation

garrettmflynn
Copy link
Collaborator

fix #9

@garrettmflynn garrettmflynn self-assigned this Jan 19, 2024
@garrettmflynn garrettmflynn marked this pull request as ready for review January 19, 2024 19:01
Comment on lines 16 to 19
for id in list(self.callbacks):
callback = self.callbacks.get(id)
if callback:
callback(self.format_dict)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for id in list(self.callbacks):
callback = self.callbacks.get(id)
if callback:
callback(self.format_dict)
for callback_id, callback in self.callbacks.items():
callback(self.format_dict)

A lot of implicit behavior here; this should make it clearer

Also outright code reduction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd originally called with list so that we don't get errors about changing the size during iteration. Will this still cover that?

Copy link
Member

Choose a reason for hiding this comment

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

How or why would running the callback on the format dict change the size of the iterator over the dictionary items during each update?

Copy link
Collaborator Author

@garrettmflynn garrettmflynn Jan 19, 2024

Choose a reason for hiding this comment

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

I came across this issue in another project, but it may require some fundamental asyncrony that isn't present here.

If you unsubscribed during the iteration—for instance, if you close the browser window during the demo—this will remove one of the callbacks. If this happens in the middle of an iteration (which would have to happen in parallel to the actual iteration...which may be the mismatch here), you will trigger an error.

You can simulate this with the following change:

for callback_id, callback in self.callbacks.items():
      callback(self.format_dict)
      self.unsubscribe(callback_id)

The original code does not throw an error because a copy is iterated over.

I wasn't able to get this to happen in practice on any of our demos, so your change will likely not have this consequence.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, my question was mainly how a call to unsubscribe on an instance of the class could ever occur while a particular call to the update method is actively running; I'm fairly sure this would be impossible with multiprocessing (even async, since that still evaluates one call at a time) since only one process would ever be responsible for handling the memory access to the attributes of that instance

Multithreading I am not so sure about since the threads would have simultaneous access to the instance and without delving deeper into the implementation details such as the GIL I can't guarantee impossibility, so let's keep this in mind going forward if we ever see a related error

I'd say keep trying to reproduce if you can (ideally in a test case the CI could capture at some level of stochasticity); it ought to be more likely to occur for a progress bar that (i) updates very frequently, (ii) runs for a relatively long time, and (iii) has either a ton of callbacks or a callbacks that for whatever reason take a very long time to run (I guess you could artificiate with a sleep delay in the callback itself before it uses the format_dict)

@CodyCBakerPhD CodyCBakerPhD merged commit 31a6d2a into main Jan 21, 2024
@CodyCBakerPhD CodyCBakerPhD deleted the docstrings branch January 21, 2024 22:35
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.

Add numpy docstrings
2 participants