make subprocess child shows different process name #391

TTianshun · 2024-01-04T15:47:21Z

The previous name for cubprocess child is MainProcess, which make it not easy to distinguish MainProcess and subprocess child.

This PR changes the name of subprocess child.

codecov-commenter · 2024-01-04T15:57:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3dd4f3f) 100.00% compared to head (a7bc758) 100.00%.
Report is 5 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      #391   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         2276      2293   +17     
=========================================
+ Hits          2276      2293   +17

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

gaogaotiantian · 2024-01-06T03:02:39Z

sys.argv[0] might be a better candidate here.

gaogaotiantian · 2024-01-07T03:08:58Z

I'm having second thought about this. This is a quick and useful change, but it also has a small side effect - other people's code might rely on multiprocessing.current_process().name and we should avoid making changes to stdlib as much as possible.

However, I agree having a different name is helpful, so let's add a variable like process_name in VizTracer or the c module and access that first when dumping json reports. If the variable is None, fall back to current solution. This way we have the least interruption to the client code.

TTianshun · 2024-01-07T16:55:15Z

I'm having second thought about this. This is a quick and useful change, but it also has a small side effect - other people's code might rely on multiprocessing.current_process().name and we should avoid making changes to stdlib as much as possible.

However, I agree having a different name is helpful, so let's add a variable like process_name in VizTracer or the c module and access that first when dumping json reports. If the variable is None, fall back to current solution. This way we have the least interruption to the client code.

OK, I can try it in the c module.

gaogaotiantian · 2024-01-21T20:28:17Z

The C module part is fine, but do not pass the process name as a cmdline argument, that's totally unnecessary. Just use your original method to set it in main() if it's a subprocess. cmdline arguments are very fragile.

Also, check the coding styles in the C code, especially spaces around else.

gaogaotiantian · 2024-01-23T06:24:35Z

Let's remove the optional command line argument to set process name, that seems a bit too much for me. In a real subprocess case, there's no way for the user to properly set it. If we don't need it internally, we don't need it.

Also, it seems like you are converting the string back and forth, from Python string to C string then back. Why not simply keep it as a Python string? You can parse that as an object and you don't need to free/reallocate memory anymore, just reference change. By parsing it as an object, you can also use None as the default value for "use other values", instead of "", which is a bit like a real value.

TTianshun · 2024-01-25T17:10:08Z

Let's remove the optional command line argument to set process name, that seems a bit too much for me. In a real subprocess case, there's no way for the user to properly set it. If we don't need it internally, we don't need it.

Also, it seems like you are converting the string back and forth, from Python string to C string then back. Why not simply keep it as a Python string? You can parse that as an object and you don't need to free/reallocate memory anymore, just reference change. By parsing it as an object, you can also use None as the default value for "use other values", instead of "", which is a bit like a real value.

Finished it. The commandline way is indeed kind of fragile.

gaogaotiantian · 2024-01-26T22:51:01Z

src/viztracer/modules/snaptrace.c

+        if (self->process_name) {
+            Py_DECREF(self->process_name);
+        }
+        self->process_name = kw_process_name;
+        Py_INCREF(self->process_name);


Suggested change

if (self->process_name) {

Py_DECREF(self->process_name);

}

self->process_name = kw_process_name;

Py_INCREF(self->process_name);

Py_INCREF(kw_process_name);

Py_XSETREF(self->process_name, kw_process_name);

Maybe also add a unicode check here - to make sure toe process name given is a string, otherwise raise an exception.

gaogaotiantian · 2024-01-26T22:51:52Z

src/viztracer/viztracer.py

@@ -43,7 +43,8 @@ def __init__(self,
                 dump_raw: bool = False,
                 sanitize_function_name: bool = False,
                 output_file: str = "result.json",
-                 plugins: Sequence[Union[VizPluginBase, str]] = []) -> None:
+                 plugins: Sequence[Union[VizPluginBase, str]] = [],
+                 process_name: Optional[str] = None) -> None:


Put process_name before output_file -> there's no strict order here, but I always feel like output_file and plugins should be at the end of the list.

gaogaotiantian · 2024-01-26T22:53:32Z

src/viztracer/modules/snaptrace.c

-            exit(-1);
+        PyObject* process_name = NULL;
+        if (self->process_name) {
+            process_name = self->process_name;


You are losing reference here. You need to increment the reference because you are decreasing it later.

gaogaotiantian · 2024-01-26T22:53:47Z

src/viztracer/modules/snaptrace.c

-        Py_DECREF(current_process_method);
-        Py_DECREF(current_process);
+        if (self->process_name) {
+            process_name = self->process_name;


New reference here as well

gaogaotiantian · 2024-01-27T20:16:56Z

tests/test_multiprocess.py

+    def test_code(self):
+        self.template(["viztracer", "-o", "result.json", "cmdline_test.py"],
+                      expected_output_file="result.json",
+                      script=file_subprocess_code)


Let's also check the process name of the subprocess on this, as it's known - should be python I believe?

Yes, the current process_name is site-packages/viztracer/__main__.py because sys.argv is processed in VizUI.run(), which is before the config.

Having a process with a name python is almost as bad as MainProcess. Let's set the process_name in run_code. Update self.init_kwargs before create the VizTracer instance. The subprocess would be distinguishable with the distinct names.

If setting process_name in run_code. For module, the process_name is module name. For run python file, the process_name is python file name. For run code by -c, the process_name is -c.

Yeah "-c" is a bit weird. Let's do another check for cmd_string and if it's set, set the process name to "python -c".

gaogaotiantian · 2024-01-30T19:54:42Z

The python -c part is not covered, probably due to the multiprocessing handling mechanism of coveragepy. Could you solve that? If it's too much for you, I can take a look at it.

TTianshun · 2024-01-31T15:20:51Z

The python -c part is not covered, probably due to the multiprocessing handling mechanism of coveragepy. Could you solve that? If it's too much for you, I can take a look at it.

Seemed that coverage didn't support subprocess as concurrency libraries. Maybe we should patch the command line for subprocess in coverage run.

gaogaotiantian · 2024-01-31T19:55:39Z

A couple of suggestions with the tests and we can be done with this.

For test_child_process, instead of checking what the process name is not, check what it is. It should be child.py right?
Use check_func argument for template for test_module. Also rename this test to test_module_process_name.
test_code is also too general, at least it should be test_code_string. It's okay at this point to use --subprocess_child for the coverage, but we should still keep the normal test that actually calls subprocess as function test. They can live in the same test method because they share the same check function. For the real test, you can also use check_func argument - it's designed for trace data check.

TTianshun · 2024-02-01T06:46:07Z

A couple of suggestions with the tests and we can be done with this.

For test_child_process, instead of checking what the process name is not, check what it is. It should be child.py right?

Use check_func argument for template for test_module. Also rename this test to test_module_process_name.

test_code is also too general, at least it should be test_code_string. It's okay at this point to use --subprocess_child for the coverage, but we should still keep the normal test that actually calls subprocess as function test. They can live in the same test method because they share the same check function. For the real test, you can also use check_func argument - it's designed for trace data check.

For test with --subprocess_child, the output file is result_{pid}.json and unable to use check_func. For test_module, I Retain the test as test_module. Because the main effect of the test is to test subprocess module.

gaogaotiantian · 2024-02-01T07:17:10Z

Great work, thanks for the contribution.

TTianshun force-pushed the change_subprocess_name branch from 0d71ee1 to 9def5f4 Compare January 6, 2024 11:27

TTianshun added 4 commits January 12, 2024 01:05

make subprocess child shows different process name

c8ab52e

use sys.argv[0] as subprocess name

07d92c0

change process_name in c module

38c75b6

fix coverage

6a5037e

TTianshun force-pushed the change_subprocess_name branch from ecc6ff9 to 6a5037e Compare January 11, 2024 17:23

TTianshun added 2 commits January 12, 2024 01:24

fix lint

4b7d72d

fix a bug

fb827cb

1. update code format 2. not use cmdline to change process_name

502208a

TTianshun added 2 commits January 25, 2024 22:36

1. remove command line arg process_name 2. save process name as PyObject

0687708

fix subprocess miss

4b476bd

gaogaotiantian reviewed Jan 26, 2024

View reviewed changes

fix refcnt error and add unicode check

71af9c0

gaogaotiantian reviewed Jan 27, 2024

View reviewed changes

TTianshun added 3 commits January 28, 2024 22:11

fix child process_name and add tests

aabb8b3

change process_name in run_code

76b26d6

change process_name of code to python -c

aa598d6

temporarily fix coverage problem

3dc0f72

gaogaotiantian mentioned this pull request Jan 31, 2024

Please provide Wheels for Apple Silicon #395

Closed

update test

a7bc758

gaogaotiantian merged commit ddaea99 into gaogaotiantian:master Feb 1, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make subprocess child shows different process name #391

make subprocess child shows different process name #391

TTianshun commented Jan 4, 2024

codecov-commenter commented Jan 4, 2024 •

edited

Loading

gaogaotiantian commented Jan 6, 2024

gaogaotiantian commented Jan 7, 2024

TTianshun commented Jan 7, 2024

gaogaotiantian commented Jan 21, 2024

gaogaotiantian commented Jan 23, 2024

TTianshun commented Jan 25, 2024

gaogaotiantian Jan 26, 2024

gaogaotiantian Jan 26, 2024

gaogaotiantian Jan 26, 2024

gaogaotiantian Jan 26, 2024

gaogaotiantian Jan 26, 2024

gaogaotiantian Jan 27, 2024

TTianshun Jan 28, 2024

gaogaotiantian Jan 28, 2024

TTianshun Jan 29, 2024

gaogaotiantian Jan 30, 2024

gaogaotiantian commented Jan 30, 2024

TTianshun commented Jan 31, 2024

gaogaotiantian commented Jan 31, 2024

TTianshun commented Feb 1, 2024

gaogaotiantian commented Feb 1, 2024

make subprocess child shows different process name #391

make subprocess child shows different process name #391

Conversation

TTianshun commented Jan 4, 2024

codecov-commenter commented Jan 4, 2024 • edited Loading

Codecov Report

gaogaotiantian commented Jan 6, 2024

gaogaotiantian commented Jan 7, 2024

TTianshun commented Jan 7, 2024

gaogaotiantian commented Jan 21, 2024

gaogaotiantian commented Jan 23, 2024

TTianshun commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaogaotiantian commented Jan 30, 2024

TTianshun commented Jan 31, 2024

gaogaotiantian commented Jan 31, 2024

TTianshun commented Feb 1, 2024

gaogaotiantian commented Feb 1, 2024

codecov-commenter commented Jan 4, 2024 •

edited

Loading