Restructure et_relay subpackage #124

amaslenn · 2024-06-17T15:53:36Z

Summary

Make et_replay structure easier to refactor: added an entry point to export public symbols, used it in train code.
Enabled tests run as part of GHA CI.

Test Plan

CI

Additional Notes

—

et_replay/ - several high level modules mainly responsible for parsing - tools # previously was on a layer above, moved here to simplify imports management - comm # comm utils and functionalities, to be expanded - __init__.py # to hide files locations yet always export important symbols `train` now doesn't care about actual files' locations.

facebook-github-bot · 2024-06-17T15:53:41Z

Hi @amaslenn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

amaslenn · 2024-06-17T15:56:13Z

cc @TaekyungHeo

facebook-github-bot · 2024-06-17T16:04:58Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

sanrise · 2024-06-19T11:55:26Z

The change is a good move to make et_replay a package. But let's keep the imports absolute, they are not long enough yet for us to make them relative. Relative imports spoil readability, pep8 friendliness and refactoring capabilities. Within this single PR we are importing the ExecutionTrace class in the following manners:
from .. import ExecutionTrace, from ..execution_trace import ExecutionTrace, from .execution_trace import ExecutionTrace.

sanrise · 2024-06-19T11:35:20Z

et_replay/pyproject.toml

@@ -7,8 +7,8 @@ name = "et_replay"
 version = "0.5.0"

 [tool.setuptools.package-dir]
-"et_replay.lib.comm" = "lib/comm"
-"et_replay.tools" = "tools"
+"et_replay" = "lib"


this makes sense, to call everything under lib as a part of the et_replay package.

sanrise · 2024-06-19T11:36:18Z

et_replay/lib/__init__.py

@@ -0,0 +1,3 @@
+from .execution_trace import ExecutionTrace


+1 on exposing the ET class from the package init, but no harm in making this import absolute.

sanrise · 2024-06-19T11:47:07Z

et_replay/lib/comm/commsTraceParser.py

+from .. import ExecutionTrace
+from . import comms_utils
+from .comms_utils import commsArgs
+from .pytorch_backend_utils import supportedP2pOps


can we keep the imports absolute? our absolute import strings are not really that long for us to justify relative imports. they also make code refactoring tricky. absolute imports are also more readable and pep8 friendly.

amaslenn · 2024-06-19T12:59:05Z

@sanrise thank you for the feedback! I mainly agree with you on relative imports topic, but there are a reason I made it like this:

Editors are going crazy: et_replay is a directory and at the same time it is a package. But the structures are different, lib doesn't exist in the package, but exists in the file structure. So I need to either keep lib for both, or use relative imports.
While relative imports have bad reputation, they are all over python's code itself. And it kinda makes sense inside packages.

Anyway, I can use absolute imports and also keep lib and yet export some symbols from init. Seems fine to me. What do you think?

facebook-github-bot · 2024-06-20T21:58:07Z

@sanrise has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

sanrise · 2024-06-20T22:52:49Z

Editors are going crazy: et_replay is a directory and at the same time it is a package. But the structures are different, lib doesn't exist in the package, but exists in the file structure. So I need to either keep lib for both, or use relative imports.

Sounds like presence of lib is vestigial at this point, and we can get rid of it. Without it, the content of lib moves up a level. This means

you don't need to patch the module path in your toml
export what you want in your __init__.py to do from et_replay import X, Y, Z
Your editor is happy.
your absolute imports stick around.

Do you see a benefit of keeping it around just to have have it remapped in the toml? I don't, and I believe this will make everything more cleaner/easier. Thoughts?

facebook-github-bot · 2024-06-24T09:55:51Z

@amaslenn has updated the pull request. You must reimport the pull request before landing.

amaslenn · 2024-06-24T09:58:39Z

Do you see a benefit of keeping it around just to have have it remapped in the toml? I don't, and I believe this will make everything more cleaner/easier. Thoughts?

Generally I prefer having a top-level folder for a package or even have src-layout. It makes packaging more standard and manageable. But in our case IMO flat structure is fine too. I've updated my PR accordingly.

sanrise · 2024-06-24T23:32:26Z

Do you see a benefit of keeping it around just to have have it remapped in the toml? I don't, and I believe this will make everything more cleaner/easier. Thoughts?

Generally I prefer having a top-level folder for a package or even have src-layout. It makes packaging more standard and manageable. But in our case IMO flat structure is fine too. I've updated my PR accordingly.

thanks a lot! will import and run the linters / static checks shortly.

facebook-github-bot · 2024-06-24T23:33:09Z

@sanrise has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

TaekyungHeo · 2024-06-24T23:45:29Z

@sanrise, please don't merge this PR until we inform you.

sanrise · 2024-06-24T23:51:59Z

et_replay/utils.py

@@ -6,7 +6,7 @@
 import uuid
 from typing import Any, Dict

-from et_replay.lib.execution_trace import ExecutionTrace
+from et_replay.execution_trace import ExecutionTrace


nit: for the record, this can also be from et_replay import ExecutionTrace. But this works too. (y)

Thank you! This makes sense, updated.

facebook-github-bot · 2024-06-25T09:50:53Z

@amaslenn has updated the pull request. You must reimport the pull request before landing.

TaekyungHeo · 2024-06-27T11:58:13Z

@sanrise , you can continue code review at #129

briancoutinho · 2024-07-12T21:21:09Z

@TaekyungHeo after import we are seeing some Black formatting error. Did you guys run black formatter already?
If yes then we might need to disable the linter either outside or in meta, if you haven't i can share the format suggestions

briancoutinho · 2024-07-12T21:21:49Z

@TaekyungHeo also can you rebase to latest param main commit?

Summary: 1. Make `et_replay` structure easier to refactor: added an entry point to export public symbols, used it in train code. 2. Enabled tests run as part of GHA CI. Reviewed at facebookresearch#124 Pull Request resolved: facebookresearch#129 Test Plan: CI ## Additional Notes -- Differential Revision: D59178710 Pulled By: sanrise

Summary: Pull Request resolved: facebookresearch#133 1. Make `et_replay` structure easier to refactor: added an entry point to export public symbols, used it in train code. 2. Enabled tests run as part of GHA CI. Reviewed at facebookresearch#124 Pull Request resolved: facebookresearch#129 Test Plan: CI ## Additional Notes -- Differential Revision: D59178710 Pulled By: sanrise

Summary: Pull Request resolved: facebookresearch#133 1. Make `et_replay` structure easier to refactor: added an entry point to export public symbols, used it in train code. 2. Enabled tests run as part of GHA CI. Reviewed at facebookresearch#124 Pull Request resolved: facebookresearch#129 Test Plan: CI ## Additional Notes -- Reviewed By: shengfukevin Differential Revision: D59178710 Pulled By: sanrise

Summary: Pull Request resolved: #133 1. Make `et_replay` structure easier to refactor: added an entry point to export public symbols, used it in train code. 2. Enabled tests run as part of GHA CI. Reviewed at #124 Pull Request resolved: #129 Test Plan: CI ## Additional Notes -- Reviewed By: shengfukevin Differential Revision: D59178710 Pulled By: sanrise fbshipit-source-id: 81762730953136d3a3560b17aac2c8b1de7e8343

amaslenn · 2024-07-17T14:13:36Z

@sanrise shall we close this PR now?

amaslenn · 2024-07-19T14:01:49Z

Let's reopen if needed.

TaekyungHeo · 2024-07-19T16:00:05Z

@amaslenn, I believe we can close this PR as it has been merged by @sanrise. #133

amaslenn added 3 commits June 17, 2024 16:44

Enabled tests, ran black

51173e2

Install dependencies

37d57e3

amaslenn requested review from kingchc, louisfeng, sunghlin, wfu-fb, shengbao-zheng and briancoutinho as code owners June 17, 2024 15:53

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 17, 2024

amaslenn changed the title ~~Restruct~~ Restructure et_relay subpackage Jun 17, 2024

More import updates + validation

1cb2414

sanrise reviewed Jun 19, 2024

View reviewed changes

amaslenn added 2 commits June 24, 2024 11:53

Get rid of lib/ to align folders and package structure

7f6ee9a

Merge branch 'main' into restruct

702a881

sanrise reviewed Jun 24, 2024

View reviewed changes

Address review comments and fix style issues

fcd6106

TaekyungHeo mentioned this pull request Jun 27, 2024

Restructure et_relay subpackage #129

Closed

amaslenn closed this Jun 27, 2024

briancoutinho reopened this Jul 12, 2024

sanrise mentioned this pull request Jul 15, 2024

Restructure et_relay subpackage (#129) #133

Closed

amaslenn closed this Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure et_relay subpackage #124

Restructure et_relay subpackage #124

amaslenn commented Jun 17, 2024

facebook-github-bot commented Jun 17, 2024

amaslenn commented Jun 17, 2024

facebook-github-bot commented Jun 17, 2024

sanrise commented Jun 19, 2024 •

edited

Loading

sanrise Jun 19, 2024

sanrise Jun 19, 2024 •

edited

Loading

sanrise Jun 19, 2024

amaslenn commented Jun 19, 2024 •

edited

Loading

facebook-github-bot commented Jun 20, 2024

sanrise commented Jun 20, 2024 •

edited

Loading

facebook-github-bot commented Jun 24, 2024

amaslenn commented Jun 24, 2024

sanrise commented Jun 24, 2024

facebook-github-bot commented Jun 24, 2024

TaekyungHeo commented Jun 24, 2024

sanrise Jun 24, 2024

amaslenn Jun 25, 2024

facebook-github-bot commented Jun 25, 2024

TaekyungHeo commented Jun 27, 2024

briancoutinho commented Jul 12, 2024

briancoutinho commented Jul 12, 2024

amaslenn commented Jul 17, 2024

amaslenn commented Jul 19, 2024

TaekyungHeo commented Jul 19, 2024

Restructure et_relay subpackage #124

Restructure et_relay subpackage #124

Conversation

amaslenn commented Jun 17, 2024

Summary

Test Plan

Additional Notes

facebook-github-bot commented Jun 17, 2024

Action Required

Process

amaslenn commented Jun 17, 2024

facebook-github-bot commented Jun 17, 2024

sanrise commented Jun 19, 2024 • edited Loading

sanrise Jun 19, 2024

Choose a reason for hiding this comment

sanrise Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

sanrise Jun 19, 2024

Choose a reason for hiding this comment

amaslenn commented Jun 19, 2024 • edited Loading

facebook-github-bot commented Jun 20, 2024

sanrise commented Jun 20, 2024 • edited Loading

facebook-github-bot commented Jun 24, 2024

amaslenn commented Jun 24, 2024

sanrise commented Jun 24, 2024

facebook-github-bot commented Jun 24, 2024

TaekyungHeo commented Jun 24, 2024

sanrise Jun 24, 2024

Choose a reason for hiding this comment

amaslenn Jun 25, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jun 25, 2024

TaekyungHeo commented Jun 27, 2024

briancoutinho commented Jul 12, 2024

briancoutinho commented Jul 12, 2024

amaslenn commented Jul 17, 2024

amaslenn commented Jul 19, 2024

TaekyungHeo commented Jul 19, 2024

sanrise commented Jun 19, 2024 •

edited

Loading

sanrise Jun 19, 2024 •

edited

Loading

amaslenn commented Jun 19, 2024 •

edited

Loading

sanrise commented Jun 20, 2024 •

edited

Loading