[build] Properly implement editable mode #8722
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8722
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 5 PendingAs of commit 9fb9ce0 with merge base 84273f4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
jackzhxng
added
the
release notes: build
larryliu0820
force-pushed
the
refactor_setup
branch
from
f0b0f35 to
ac4dddb
Compare
GregoryComer
left a comment
GregoryComer
left a comment
There was a problem hiding this comment.
Looks great. Thanks for fixing this.
| "pip", | ||
| "install", | ||
| ] | ||
| + (["--editable"] if args.editable else []) |
There was a problem hiding this comment.
Thoughts on making this default? Or at least have a PR in draft mode to test the whole CI pipeline with editable?
| return | ||
|
|
||
| if self.editable_mode: | ||
| self._ran_build = True |
There was a problem hiding this comment.
Don't you need to set _ran_build = True in non editable mode too?
There was a problem hiding this comment.
No non-editable mode will trigger build by default so we don't have to trigger build in build_ext
| file from the build directory to the correct location in the local | ||
| directory. | ||
| This should only be triggered when inplace mode (editable mode) is enabled. |
There was a problem hiding this comment.
This should only be triggered when inplace mode (editable mode) is enabled.
Where's the code that enforces that?
There was a problem hiding this comment.
In setuptools build_ext (base class) https://github.com/pypa/setuptools/blob/main/setuptools/command/build_ext.py#L96-L102
| if isinstance(ext, BuiltExtension): | ||
| modpath = ext.name.split(".") | ||
| package = ".".join(modpath[:-1]) | ||
| package_dir = os.path.abspath(build_py.get_package_dir(package)) | ||
| else: | ||
| # HACK: get rid of the leading "executorch" in ext.dst. | ||
| # This is because we don't have a root level "executorch" module. | ||
| package_dir = ext.dst.removeprefix("executorch/") | ||
|
|
||
| # Ensure that the destination directory exists. | ||
| self.mkpath(os.fspath(package_dir)) | ||
|
|
||
| regular_file = ext.src_path(self) | ||
| inplace_file = os.path.join( | ||
| package_dir, os.path.basename(ext.src_path(self)) | ||
| ) |
There was a problem hiding this comment.
Can you comment what's happening here?
There was a problem hiding this comment.
See my PR description:
* Copy the built extensions (shared libraries or binaries) to the correct inplace locations.
* For example, for normal install flatc will be installed to <site-packages>/executorch/data/bin but in editable it goes to executorch/data/bin.
* Similarly <site-packages>/executorch/extension/llm/custom_ops/libcustom_ops_aot_lib.[dylib|so] goes into executorch/extension/llm/custom_ops/libcustom_ops_aot_lib.[dylib|so].
|
In your Test Plan, can you comment an actual workflow of editable mode?
|
|
@malfet - can you take a look at this PR too to see anything obvious we're missing? |
This looks OK to me, though personally I still do |
Summary
Fixes #2871 #8379
This PR fixes a bunch of issues:
buildcommand inbuild_extto make sure the extensions are built.inplacelocations.flatcwill be installed to<site-packages>/executorch/data/binbut in editable it goes toexecutorch/data/bin.<site-packages>/executorch/extension/llm/custom_ops/libcustom_ops_aot_lib.[dylib|so]goes intoexecutorch/extension/llm/custom_ops/libcustom_ops_aot_lib.[dylib|so].PYTHONPATHcontains the site package path of whatever virtual environment wheresetuptoolsis being run from.executorch/extension/llm/custom_ops/custom_ops.pyfind the library so that it can find the library in both normal build mode and editable build mode.Test plan
Added CI jobs to
pull.ymlandtrunk.yml.