Updates for @log_sparse with context manager and parameters for %%viztracer #360

AlexeyKozhevin · 2023-08-04T16:35:45Z

Closes log_sparse=True doesn't work with context manager #359
Also allows to define functions decorated by @log_sparse outside of context manager
Add parameters for %%viztracer
Extend tests
Add shield_ignore method to reset counter of the ignored stack calls

gaogaotiantian

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.

gaogaotiantian · 2023-08-13T23:08:44Z

src/viztracer/cellmagic.py

@@ -32,6 +36,40 @@ def view():  # pragma: no cover

            display(button)

+        def make_kwargs(self, line, tracer_signature, viewer_signature):


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.

gaogaotiantian · 2023-08-13T23:12:12Z

src/viztracer/decorator.py

            return ret

        return wrapper
+
+
+def _log_sparse_wrapper_with_args(stack_depth: int = 0, dynamic_tracer_check: bool = False) -> Callable:


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-commenter · 2023-08-13T23:25:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3ecd46a) 100.00% compared to head (27d5789) 100.00%.
Report is 1 commits behind head on master.

❗ 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

Files	Coverage Δ
src/viztracer/cellmagic.py	`100.00% <100.00%> (ø)`
src/viztracer/decorator.py	`100.00% <100.00%> (ø)`
src/viztracer/tracer.py	`100.00% <100.00%> (ø)`
src/viztracer/viztracer.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gaogaotiantian

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.

src/viztracer/cellmagic.py

src/viztracer/decorator.py

gaogaotiantian · 2023-10-01T22:07:25Z

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.

AlexeyKozhevin · 2023-10-02T08:32:48Z

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

gaogaotiantian · 2023-10-02T18:31:55Z

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.

gaogaotiantian

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.

AlexeyKozhevin · 2023-10-24T09:12:25Z

Sorry, I really forgot one file. Thank you for comments, I've updated basic_usage.rst.
I leave VizShield in a separate file because the very similar and small VizEvent is in a separate file. The only difference I see is that VizShield is not for users in general. Perhaps they should be put in the same file with context managers?

gaogaotiantian · 2023-10-30T01:52:44Z

Sorry, I really forgot one file. Thank you for comments, I've updated basic_usage.rst. I leave VizShield in a separate file because the very similar and small VizEvent is in a separate file. The only difference I see is that VizShield is not for users in general. Perhaps they should be put in the same file with context managers?

I don't think we should have a VizShield class. The shield feature will only used in VizTracer class and it should be so simple with setignorestackcounter(). The current structure is over-complicated. VizEvent might not be the best design, but the user can directly use it.

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 VizShield in a file that does not use it which caused a lint error. Should be trivial to fix.

gaogaotiantian · 2023-10-30T18:09:16Z

One final last thing and we are done. Could you move from contextlib import contextmanager before from typing import Any, Callable, Dict, Optional, Sequence, Tuple, Union? Or, feel free to do import contextlib and use contextlib.contextmanager if that's what you prefer.

We are grouping import XX and from XX import YY separately which is what stdlib does (mostly).

AlexeyKozhevin · 2023-10-31T20:00:36Z

I conducted more detailed testing and ran local tests. Unfortunately, I have to return to _VizShield decorator due to additional redundant calls.

At the last commit implementation we need to compensate stack of calls by adding or subtracting of 2 in setignorestackcounter and by adding 1 in the __enter__ of _VizShield (see comments in the code)

In the case of using @contextlib we have to work with additional generator and next calls (see image below). The generator makes the logic of calls compensation not so straightforward and I haven't figured out how to handle it (just increasing the setignorestackcounter parameter doesn't work).

I concluded that syntactic sugar of contexlib makes it difficult to understand what shield_ignore does and stopped at the current implementation. What do you think about it?

Also wondering what you think about the _VizShield place? I haven't found any places where classes are defined locally, but here the class is purely disposable, so I thought it would be appropriate to define it inside shield_ignore

gaogaotiantian · 2023-10-31T20:33:18Z

Ah, right. The with command is actually a series of calls before/after the statement. In that case, I'd say just get rid of the with command. The compensation is easier to understand with the class implementation, but still not easy enough - it's fragile.

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 setignorestackcounter is not logged in the trace? Basically a test that would fail if you did not do the compensation in setignorestackcounter correctly - that would be helpful in the future to prevent regression.

AlexeyKozhevin · 2023-10-31T21:25:03Z

Do you mean

    def shield_ignore(self, func, *args, **kwargs):
        prev_ignore_stack = self.setignorestackcounter(0)
        res = func(*args, **kwargs)
        self.setignorestackcounter(prev_ignore_stack)
        return res

and

    return shield_ignore(func, *args, **kwargs))

instead of

with local_tracer.shield_ignore():
    return func(*args, **kwargs)

?

And yes, I will add a test.

gaogaotiantian · 2023-10-31T21:34:28Z

Yes that's what I meant - also makes to code simpler I believe.

AlexeyKozhevin · 2023-11-01T15:25:37Z

I have extended the tests and also modified the existing test_basic. Since entry["name"] for the function is not just f, but also the path to the module where it is defined, the check in check_func always passed. And it seems that in general the wrong things were checked there :)

gaogaotiantian · 2023-11-05T06:58:03Z

Okay now we only have a couple of lint issues - you can install flake8 and mypy in your environment and do make lint to check it quickly. For the cellmagic.py, I don't know what happened there, feel free to figure it out, but if it's too complicated, just do an inline ignore (I think it's # type: ignore? There are a couple in the source files already).

For the decorator, I think _log_sparse_wrapper is guaranteed a not None func right? So removing the Optional for the func argument typing might fix most of the problems.

If you are not familiar with typing, you can send the typing you can't solve here and I'll take a look. Thanks!

AlexeyKozhevin · 2023-11-07T15:05:51Z

flake8 and mypy say it's okay now.

Unfortunately, I have no idea what is the problem with cellmagic.py so I have to set # type: ignore.

gaogaotiantian · 2023-11-08T00:42:32Z

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!

AlexeyKozhevin · 2023-11-08T08:09:47Z

Thank you for advices and review! It was really interesting.
We already use viztracer in our ML projects in notebook. Some things have become much easier to profile

AlexeyKozhevin added 7 commits August 4, 2023 16:04

Fix log_sparse for context manager

61b5e0f

Allow to define log_sparse functions outside of decorator

7039e19

Add dynamic_tracer_check parameter

526e700

Update documentation

82680a2

Add kwargs to %%viztracer

7fb9dcb

Fix log_sparse=False mode

346c621

Update docstrings

7e91b53

AlexeyKozhevin changed the title ~~Fix log sparse~~ Updates for @log_sparse with contect manager and parameters for %%viztracer Aug 10, 2023

Add strings processing for magic

f9d60b8

gaogaotiantian requested changes Aug 13, 2023

View reviewed changes

AlexeyKozhevin added 3 commits August 14, 2023 16:50

Changes in magic args

d602260

Update tests; Add setignorestackcounter

fc54687

Remove some arguments from magic

d1838fa

AlexeyKozhevin requested a review from gaogaotiantian August 17, 2023 14:53

gaogaotiantian requested changes Aug 22, 2023

View reviewed changes

src/viztracer/cellmagic.py Outdated Show resolved Hide resolved

src/viztracer/decorator.py Outdated Show resolved Hide resolved

AlexeyKozhevin added 3 commits October 23, 2023 16:04

Add VizShield

7e94945

Merge

5d9dbd5

Remove verbose parameter of magic

3f51ae8

AlexeyKozhevin requested a review from gaogaotiantian October 23, 2023 16:32

gaogaotiantian requested changes Oct 24, 2023

View reviewed changes

Add vizshield.py; Update docs

0d12d89

AlexeyKozhevin requested a review from gaogaotiantian October 26, 2023 07:59

Changes in shield

cb1b6f4

Return _VizShield

ca35f7c

Update tests for log_sparse

06abbc3

AlexeyKozhevin changed the title ~~Updates for @log_sparse with contect manager and parameters for %%viztracer~~ Updates for @log_sparse with context manager and parameters for %%viztracer Nov 1, 2023

Fix linter

27d5789

gaogaotiantian merged commit 704005f into gaogaotiantian:master Nov 8, 2023
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for @log_sparse with context manager and parameters for %%viztracer #360

Updates for @log_sparse with context manager and parameters for %%viztracer #360

AlexeyKozhevin commented Aug 4, 2023 •

edited

Loading

gaogaotiantian left a comment

gaogaotiantian Aug 13, 2023

gaogaotiantian Aug 13, 2023

codecov-commenter commented Aug 13, 2023 •

edited

Loading

gaogaotiantian left a comment

gaogaotiantian commented Oct 1, 2023

AlexeyKozhevin commented Oct 2, 2023

gaogaotiantian commented Oct 2, 2023

gaogaotiantian left a comment

AlexeyKozhevin commented Oct 24, 2023 •

edited

Loading

gaogaotiantian commented Oct 30, 2023 •

edited

Loading

gaogaotiantian commented Oct 30, 2023

AlexeyKozhevin commented Oct 31, 2023

gaogaotiantian commented Oct 31, 2023

AlexeyKozhevin commented Oct 31, 2023

gaogaotiantian commented Oct 31, 2023

AlexeyKozhevin commented Nov 1, 2023 •

edited

Loading

gaogaotiantian commented Nov 5, 2023

AlexeyKozhevin commented Nov 7, 2023 •

edited

Loading

gaogaotiantian commented Nov 8, 2023

AlexeyKozhevin commented Nov 8, 2023

		@@ -32,6 +36,40 @@ def view(): # pragma: no cover

		display(button)

		def make_kwargs(self, line, tracer_signature, viewer_signature):

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

AlexeyKozhevin commented Aug 4, 2023 • edited Loading

gaogaotiantian left a comment

Choose a reason for hiding this comment

gaogaotiantian Aug 13, 2023

Choose a reason for hiding this comment

gaogaotiantian Aug 13, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 13, 2023 • edited Loading

Codecov Report

gaogaotiantian left a comment

Choose a reason for hiding this comment

gaogaotiantian commented Oct 1, 2023

AlexeyKozhevin commented Oct 2, 2023

gaogaotiantian commented Oct 2, 2023

gaogaotiantian left a comment

Choose a reason for hiding this comment

AlexeyKozhevin commented Oct 24, 2023 • edited Loading

gaogaotiantian commented Oct 30, 2023 • edited Loading

gaogaotiantian commented Oct 30, 2023

AlexeyKozhevin commented Oct 31, 2023

gaogaotiantian commented Oct 31, 2023

AlexeyKozhevin commented Oct 31, 2023

gaogaotiantian commented Oct 31, 2023

AlexeyKozhevin commented Nov 1, 2023 • edited Loading

gaogaotiantian commented Nov 5, 2023

AlexeyKozhevin commented Nov 7, 2023 • edited Loading

gaogaotiantian commented Nov 8, 2023

AlexeyKozhevin commented Nov 8, 2023

AlexeyKozhevin commented Aug 4, 2023 •

edited

Loading

codecov-commenter commented Aug 13, 2023 •

edited

Loading

AlexeyKozhevin commented Oct 24, 2023 •

edited

Loading

gaogaotiantian commented Oct 30, 2023 •

edited

Loading

AlexeyKozhevin commented Nov 1, 2023 •

edited

Loading

AlexeyKozhevin commented Nov 7, 2023 •

edited

Loading