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

Remove vestigial debug print statement in walk_head_nodes #10718

Merged
merged 3 commits into from
May 2, 2022

Conversation

shadeMe
Copy link
Contributor

@shadeMe shadeMe commented Apr 27, 2022

Description

Removes one of the last, potential GIL-Refnanny related regressions.

Types of change

Bug fix

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@shadeMe shadeMe requested a review from svlandeg April 27, 2022 16:45
@shadeMe shadeMe added bug Bugs and behaviour differing from documentation perf / speed Performance: speed and removed bug Bugs and behaviour differing from documentation labels Apr 27, 2022
@adrianeboyd
Copy link
Contributor

I realize that it's a really small change, but all the reformatting makes this PR kind of hard to follow and is going to make the history really confusing in the future. (I had to use a text search to find the place where the print statement was removed in the commit.)

We don't have automatic processes for formatting cython, so if there's a good solution that you're aware of, it would be useful to add it. If it's not, then I would prefer if this kind of PR didn't do any unrelated reformatting, or at least not beyond a few lines at most. What does everyone think?

@shadeMe
Copy link
Contributor Author

shadeMe commented Apr 27, 2022

Ah, I didn't notice all the whitespace changes on my end. I can revert them, not an issue. That said, the GitHub diff interface does let one hide whitespace changes: Click on the gear icon at the top of the Files Changed page, check "Hide whitespace" and hit Apply.

I don't have a specific formatter for Cython, but my editor is configured to automatically trim trailing whitespaces (which is what happened here).

@svlandeg
Copy link
Member

I realize that it's a really small change, but all the reformatting makes this PR kind of hard to follow and is going to make the history really confusing in the future. (I had to use a text search to find the place where the print statement was removed in the commit.)

We don't have automatic processes for formatting cython, so if there's a good solution that you're aware of, it would be useful to add it. If it's not, then I would prefer if this kind of PR didn't do any unrelated reformatting, or at least not beyond a few lines at most. What does everyone think?

Agreed! As long as we don't have a standard way of formatting Cython, I'd avoid making too many formatting changes in parts of the code that would otherwise be untouched by a PR. Mostly for git blame's sake ;-)

Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

The graph.pyx stuff is not currently used by anyone or anything, as far as I know. There's actually plenty of other print statements that should be cleaned up in this same file, or at least put behind some debug variable or something.

The fact that that didn't happen yet though, makes me wonder whether this was still a WIP that still required some further debugging & experimentations first...

@shadeMe
Copy link
Contributor Author

shadeMe commented Apr 28, 2022

I only see two other print statements in the graph.pyx file on my end. Both of them are in regular Python functions, so they shouldn't concern us at least with respect to RefNanny. Still, I could remove them in this PR if that's what we want to do (feature-gating them seems like overkill in this case given how ad-hoc they appear to be).

In any event, I do think it makes sense to merge the proposed change as it will eliminate a potential bottleneck in the future, especially given that the GIL lock was being acquired in a graph traversal method.

@svlandeg
Copy link
Member

Yea, let's just remove the other print statements too (you're right that there are currently 3 in total, I seemed to recall there were more)

@adrianeboyd adrianeboyd merged commit 0a503ce into explosion:master May 2, 2022
@shadeMe shadeMe deleted the fix/gil-refnanny branch May 2, 2022 11:39
danieldk pushed a commit that referenced this pull request Jun 7, 2022
* `graph`: Remove vestigial debug print statement in `walk_head_nodes`

* Revert whitespace changes

* Remove more debug print statements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf / speed Performance: speed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants