Support PPE profiling for `Local` #340

KantaTamura · 2024-08-22T02:41:13Z

This PR resolves the Local task in #258

Note

To enable tracing, trace=True must be added during Local() initialization (or from_url()).

Sample program to check tracing

import json
import pytorch_pfn_extras as ppe

from pfio.v2 import Local, Path, from_url

@ppe.profiler.record_function("my_tag_2", trace=True)
def my_run2(x):
    pass

def my_run1(x):
    pass

def p():
    tracer = ppe.profiler.get_tracer()

    tracer.clear()
    with ppe.profiler.record("my_tag_1", trace=True):
        my_run1('hoge')

    for i in range(4):
        my_run2("far")

    dir = 0
    fil = 0
    with Local(trace=True) as fs:
        for f in fs.list():
            if fs.isdir(f.strip()):
                dir += 1
                continue
            
            fil += 1
            len(fs.open(f).read())

    print(fil, "files", dir, "dirs")

    dict = tracer.state_dict()
    keys = [event["name"] for event in json.loads(dict["_event_list"])]
    print(keys)

    w = ppe.writing.SimpleWriter(out_dir="")

    # output '['
    tracer.initialize_writer("trace.json", w)
    # output json dump
    tracer.flush("trace.json", w)

p()

output

9 files 15 dirs
['my_tag_1', 'my_tag_2', 'my_tag_2', 'my_tag_2', 'my_tag_2', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-0', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-1', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-2', 'pfio.v2.Local:list-3', 'pfio.v2.Local:list-4', 'pfio.v2.Local:list-5', 'pfio.v2.Local:list-6', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-7', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-8', 'pfio.v2.Local:list-9', 'pfio.v2.Local:list-10', 'pfio.v2.Local:list-11', 'pfio.v2.Local:list-12', 'pfio.v2.Local:list-13', 'pfio.v2.Local:list-14', 'pfio.v2.Local:list-15', 'pfio.v2.Local:list-16', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-17', 'pfio.v2.Local:list-18', 'pfio.v2.Local:list-19', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-20', 'pfio.v2.Local:list-21', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-22', 'pfio.v2.Local:open', 'pfio.v2.Local:read', 'pfio.v2.Local:list-23']

drawing of the output json file (trace.json) with chrome://tracing is shown below

Can be specified in the `trace` argument when creating a Local() instance.

k5342

Overall LGTM. I have one minor suggestion.

k5342 · 2024-08-22T06:49:32Z

setup.py

@@ -52,7 +52,7 @@
    packages=find_packages(),
    package_data={'pfio': package_data},
    extras_require={
-        'test': ['pytest', 'flake8', 'autopep8', 'parameterized', 'isort', 'moto[server]>=5.0.0', 'numpy', 'mypy'],


We can also place the pytorch-pfn-extras package under the extras_require section by adding new presets (e.g., trace), not only for testing purposes.

Agree. We'd rather add something like trace than adding the dependency to test. PFIO users shall be using profiling without this extra requires and will be installing PFIO with pip install pfio . In that case, PPE won't be installed even though they need it.

k5342 · 2024-08-22T07:15:08Z

@kuenishi Could you take a look? Please let us know if you have any comments on the tag naming rule and the granularity of the tests.

kuenishi

The Python implementation itself looks good, but I have a few request to align with current style and options.

kuenishi · 2024-08-22T07:41:13Z

setup.py

@@ -52,7 +52,7 @@
    packages=find_packages(),
    package_data={'pfio': package_data},
    extras_require={
-        'test': ['pytest', 'flake8', 'autopep8', 'parameterized', 'isort', 'moto[server]>=5.0.0', 'numpy', 'mypy'],


Agree. We'd rather add something like trace than adding the dependency to test. PFIO users shall be using profiling without this extra requires and will be installing PFIO with pip install pfio . In that case, PPE won't be installed even though they need it.

kuenishi · 2024-08-22T07:45:13Z

tests/v2_tests/test_local.py

@@ -291,6 +293,45 @@ def test_from_url_create_option(self):
        from_url(path, create=True)
        assert os.path.exists(path) and os.path.isdir(path)

+    def test_ppe_profiling(self):


Please don't extend the TestLocal(unittest.TestCase) class. We're slowly moving on from the unitest style, to a more pytest-like style where each test is defined as a single and independent function (sometimes with fixtures or parameterized). Please refer to test_s3.py.

Changed to not use TestLocal(unittest.TestCase).

Currently, foo.txt is generated in the directory where pytest runs, do you want to use a different path? (e.g. /tmp/...)

kuenishi · 2024-08-22T07:49:16Z

pfio/v2/local.py

@@ -4,9 +4,54 @@
 import shutil
 from typing import Optional

+try:


Refer to: #326 (comment)

Split into pfio/v2/_profiler.py and changed to import from _profiler.py.

kuenishi · 2024-08-23T10:09:27Z

setup.py

@@ -52,11 +52,12 @@
    packages=find_packages(),
    package_data={'pfio': package_data},
    extras_require={
-        'test': ['pytest', 'flake8', 'autopep8', 'parameterized', 'isort', 'moto[server]>=5.0.0', 'numpy', 'mypy'],
+        'test': ['pytest', 'flake8', 'autopep8', 'parameterized', 'isort', 'moto[server]>=5.0.0', 'numpy', 'mypy', 'pytorch-pfn-extras'],


As we discussed offline, PPE shouldn't be here but tests must be switched in the test code using something like skipif.

Changed to using importorskip() to skip if no ppe.

no ppe

test_local.py ................s

exist ppe

test_local.py .................

We may also need to tweak the tox.ini to enable CI testing for the tracing code, which we're adding this PR if we remove PPE from the test section in setup.py:

pfio/tox.ini

Lines 4 to 20 in b874d49

[testenv]

deps = .[test]

skipsdist = True

commands =

flake8 pfio tests

autopep8 -r pfio tests --diff

isort . --check --diff

mypy pfio

pytest -W error::deprecation.UnsupportedWarning tests -s -v

[testenv:doc]

deps = .[doc]

skipsdist = True

changedir = docs

commands =

make html

allowlist_externals=make

kuenishi · 2024-08-26T05:00:13Z

Offline discussion summary: the code itself looks nice, but the example script at the issue description doesn't provide valid JSON file and I'm not sure how to verify the output JSON(-like) file by loading onto Chrome tracing. After a minimal script to test this patch provided, I'll revisit and retry validation.

kuenishi

Please leave the summary of offline discussion with PPE developer why we don't need complete JSON file and it works fine without it.

kuenishi

Ah, and don't forget to update tox.ini to include trace option as @k5342 pointed out.

KantaTamura · 2024-08-26T06:09:56Z

Offline discussion summary: Chrome Tracing does not require valid json and can be rendered with just initialize_writer() and flush() as in the sample program.

ref. Trace Event Format

When provided as a string to the importer the ] at the end of the JSON Array Format is optional.

KantaTamura · 2024-08-26T06:28:23Z

fix tox.ini and summarized the json output method in the documentation (tips).

* Profiling with PPE * Profiling with PPE * Make mypy happy * Make flake8 happy * Make isort and mypy even happier * Support all methods * Align tag naming rule with PPE (module.Klass:method) * Support PPE tracing * Local: Support io class method * Add trace switching Can be specified in the `trace` argument when creating a Local() instance. * Split PPE methods * mypy: ignore ppe * add trace test * setup: add trace context * doc: add output tracer results tips --------- Co-authored-by: UENISHI Kota <[email protected]>

kuenishi added 7 commits August 21, 2024 11:27

Profiling with PPE

3975b29

Profiling with PPE

b35a47a

Make mypy happy

b3ba64d

Make flake8 happy

a723f1b

Make isort and mypy even happier

dd0dd1c

Support all methods

338afd9

Align tag naming rule with PPE (module.Klass:method)

41e6a3b

KantaTamura force-pushed the local-profile branch 4 times, most recently from 4974a73 to df1c1d3 Compare August 22, 2024 04:23

KantaTamura added 3 commits August 22, 2024 13:27

Support PPE tracing

36a6fca

Local: Support io class method

af772bf

Add trace switching

7cc4b58

Can be specified in the `trace` argument when creating a Local() instance.

KantaTamura force-pushed the local-profile branch from df1c1d3 to 8fef411 Compare August 22, 2024 04:28

k5342 reviewed Aug 22, 2024

View reviewed changes

k5342 requested a review from kuenishi August 22, 2024 07:10

kuenishi requested changes Aug 22, 2024

View reviewed changes

kuenishi added the cat:feature Implementation that introduces new interfaces. label Aug 22, 2024

kuenishi modified the milestones: 2.8.0, 2.9.0 Aug 22, 2024

kuenishi reviewed Aug 22, 2024

View reviewed changes

KantaTamura force-pushed the local-profile branch 4 times, most recently from 4301abd to 87e3eaf Compare August 23, 2024 07:57

kuenishi reviewed Aug 23, 2024

View reviewed changes

KantaTamura force-pushed the local-profile branch from 87e3eaf to e6c128c Compare August 26, 2024 00:00

Split PPE methods

0ffb9ae

KantaTamura force-pushed the local-profile branch from e6c128c to c40bdba Compare August 26, 2024 00:01

KantaTamura added 2 commits August 26, 2024 09:16

mypy: ignore ppe

1956179

add trace test

62d4b0b

KantaTamura force-pushed the local-profile branch from 98259ec to 0c1d1e1 Compare August 26, 2024 00:16

kuenishi approved these changes Aug 26, 2024

View reviewed changes

setup: add trace context

033d420

kuenishi requested changes Aug 26, 2024

View reviewed changes

doc: add output tracer results tips

19f5bae

KantaTamura force-pushed the local-profile branch from 0c1d1e1 to 19f5bae Compare August 26, 2024 06:19

kuenishi approved these changes Aug 26, 2024

View reviewed changes

kuenishi merged commit bdc81e5 into pfnet:master Aug 26, 2024
7 checks passed

kuenishi mentioned this pull request Aug 26, 2024

Profiling with PPE for Local #326

Closed

kuenishi mentioned this pull request Oct 17, 2024

Support PPE profiling #258

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PPE profiling for `Local` #340

Support PPE profiling for `Local` #340

KantaTamura commented Aug 22, 2024 •

edited

Loading

k5342 left a comment

k5342 Aug 22, 2024

kuenishi Aug 22, 2024

k5342 commented Aug 22, 2024

kuenishi left a comment

kuenishi Aug 22, 2024

kuenishi Aug 22, 2024

KantaTamura Aug 23, 2024

kuenishi Aug 22, 2024

KantaTamura Aug 23, 2024

kuenishi Aug 23, 2024

KantaTamura Aug 26, 2024

k5342 Aug 26, 2024

kuenishi Aug 26, 2024

kuenishi commented Aug 26, 2024

kuenishi left a comment

kuenishi left a comment

KantaTamura commented Aug 26, 2024

KantaTamura commented Aug 26, 2024

	[testenv]
	deps = .[test]
	skipsdist = True
	commands =
	flake8 pfio tests
	autopep8 -r pfio tests --diff
	isort . --check --diff
	mypy pfio
	pytest -W error::deprecation.UnsupportedWarning tests -s -v

	[testenv:doc]
	deps = .[doc]
	skipsdist = True
	changedir = docs
	commands =
	make html
	allowlist_externals=make

Support PPE profiling for Local #340

Support PPE profiling for Local #340

Conversation

KantaTamura commented Aug 22, 2024 • edited Loading

k5342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k5342 commented Aug 22, 2024

kuenishi left a 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

Choose a reason for hiding this comment

kuenishi commented Aug 26, 2024

kuenishi left a comment

Choose a reason for hiding this comment

kuenishi left a comment

Choose a reason for hiding this comment

KantaTamura commented Aug 26, 2024

KantaTamura commented Aug 26, 2024

Support PPE profiling for `Local` #340

Support PPE profiling for `Local` #340

KantaTamura commented Aug 22, 2024 •

edited

Loading