-
-
Notifications
You must be signed in to change notification settings - Fork 379
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
Updates for @log_sparse with context manager and parameters for %%viztracer #360
Updates for @log_sparse with context manager and parameters for %%viztracer #360
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the docs and the code is good. I have left some comments for the part that I personally have some concerns. Also as I pursue 100% coverage for viztracer, there may or may not be some extra tests requirement - let's see the coverage result.
src/viztracer/cellmagic.py
Outdated
@@ -32,6 +36,40 @@ def view(): # pragma: no cover | |||
|
|||
display(button) | |||
|
|||
def make_kwargs(self, line, tracer_signature, viewer_signature): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You basically wrote an argument parser here, which would be difficult to maintain and requires a lot of tests (viztracer keeps a 100% coverage). I don't think this should be the way to go, especially considering there could be conflict arguments (for example, eliminating --once
is a very bad idea for the viewer).
I don't want the magic cells to have full flexibility as the CLI, that could be unnecessarily difficult to maintain. However, having some options like --port
might be beneficial. Still, I prefer a white list way instead of this auto-importing kind of way (this implementation will introduce a silent dependency that VizTracer and ServerThead can't share args with the same name, which is not ideal).
To be honest, cell magic is not the top-priority for viztracer - it would be nice if it works, but it's totally fine that it won't work under some circumstances. To leverage its full capability, the users can always use the inline VizTracer
context and open the report with code.
So, I think I'd prefer this piece to be removed, and if you want some useful argument for the cell magic, use something like IPython.core.magic_arguments
, which we do not need to test for its validity.
src/viztracer/decorator.py
Outdated
return ret | ||
|
||
return wrapper | ||
|
||
|
||
def _log_sparse_wrapper_with_args(stack_depth: int = 0, dynamic_tracer_check: bool = False) -> Callable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only used once and the content is a single line. I don't see a very strong reason to abstract it out. I think we can just return the partial
when the func
is None
in log_sparse
.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #360 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 2209 2231 +22
=========================================
+ Hits 2209 2231 +22
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic has became a bit hard to follow. I made a structural suggestion for the current logic, feel free to discuss it if you disagree.
Hi @AlexeyKozhevin , are you still interested in working on this PR? I can take over if you think this is too much work. I think the PR still has its value and I'd like to merge it once it's polished. |
Hi, unfortunately, I haven't had enough time lately, but I expect to get back to the task in the next two weeks. If anything changes, I will post. Thank you very much for your interest in PR |
Sounds good! Thanks for your time. Python 3.12.0 is coming soon so I plan to update viztracer to 0.16.0 (there are a lot of payloads for the version now) and support 3.12. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
Seems like you forgot to add the newly added file? I'm not sure if we want a new file and a new class. Is it possible to implement the shield logic in the function directly with contextmanager
? I would expect the logic to be rather straightforward.
Also, you made it possible to pass arguments to %%viztracer
, which is great. The user does not know that unfortunately. Could you add something in the docs, after the jupyter part, just mention that some of the command line arguments are supported, and list the supported arguments? Thanks!
Other than that, I think the current code looks good.
Sorry, I really forgot one file. Thank you for comments, I've updated |
I don't think we should have a I think the shield code can be as simple as from contextlib import contextmanager
...
@contextmanager
def shield_ignore(self):
prev_ignore_stack = self.setignorestackcounter(0)
try:
yield
finally:
self.setignorestackcounter(prev_ignore_stack) I don't think you need to care about whether the tracer is enabled or in log_sparse mode, shield is shield - the caller is responsible for checking the environment. Also you imported |
One final last thing and we are done. Could you move We are grouping |
Ah, right. The Let's do def shield_ignore(self, func, *args, **kwargs):
prev_counter = self.setignorestackcounter(0)
func(*args, **kwargs)
self.setignorestackcounter(prev_counter) Could you write a test to make sure |
Do you mean
and
instead of
? And yes, I will add a test. |
Yes that's what I meant - also makes to code simpler I believe. |
I have extended the tests and also modified the existing |
Okay now we only have a couple of lint issues - you can install For the decorator, I think If you are not familiar with typing, you can send the typing you can't solve here and I'll take a look. Thanks! |
Unfortunately, I have no idea what is the problem with |
Thank you for your contribution, it's been a long journey and I'm really glad that log_sparse now works with the inline tracer and jupyter! |
Thank you for advices and review! It was really interesting. |
log_sparse=True
doesn't work with context manager #359@log_sparse
outside of context manager%%viztracer
shield_ignore
method to reset counter of the ignored stack calls