Skip to content

Rename top-level build folder to simplify wheel building #9117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jathu opened this issue Mar 11, 2025 · 13 comments
Closed

Rename top-level build folder to simplify wheel building #9117

jathu opened this issue Mar 11, 2025 · 13 comments
Assignees
Labels
actionable Items in the backlog waiting for an appropriate impl/fix module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch module: ci Issues related to continuous integration
Milestone

Comments

@jathu
Copy link
Contributor

jathu commented Mar 11, 2025

Based on the python docs, and the PyTorch wheel building workflow, building a wheel should just take the following steps:

# Installs deps referenced in pyproject.toml → build-system.requires
$ python3 -m pip install --upgrade build

# Builds the wheel
$ python3 -m build --wheel

Unfortunately, since we have a top-level folder called build, I think python is attempting to use that as a module — which is invalid. If we rename the folder from build, we should be able to simplify our CI setup to just those steps.

Although, the effort to rename the folder seems large and daunting. Is it worth the simplified workflow?

cc @larryliu0820 @lucylq

@jathu jathu added module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch module: ci Issues related to continuous integration labels Mar 11, 2025
@GregoryComer
Copy link
Member

GregoryComer commented Mar 11, 2025

I'm in favor of this. When I first started working on ET, I ended up doing cmake builds in the build directory without thinking multiple times and making a mess. Renaming our current build/ dir seems like a good idea.

@mergennachin
Copy link
Contributor

The effort to rename the folder seems large and daunting. Is it worth the simplified workflow?

Why is this large and daunting? Isn't this a one-time codemod in fbsource?

@jathu
Copy link
Contributor Author

jathu commented Mar 11, 2025

Why is this large and daunting? Isn't this a one-time codemod in fbsource?

We have to capture all explicit usage of build/, but there might be also some implicit references. Not impossible, but would require a back and forth on updating, then letting the CI provide some feedback.

However, the bigger impact would be to update our docs that reference build scripts under that folder. This will also make any external tutorials stale since those scripts will not exist. Not a huge problem, but is it worth the effort to reduce wheel building by 1-2 steps?

@larryliu0820
Copy link
Contributor

Installs deps referenced in pyproject.toml → build-system.requires

$ python3 -m pip install --upgrade build

Builds the wheel

$ python3 -m build --wheel

We might be able to ask pip to look into a different folder than build. We are currently using pip-out so maybe we can configure pip to look into pip-out?

@jathu
Copy link
Contributor Author

jathu commented Mar 11, 2025

We might be able to ask pip to look into a different folder than build. We are currently using pip-out so maybe we can configure pip to look into pip-out?

The problem is build is a module, and it's conflicting with our top-level folder name build when running python3 -m build --wheel (this essentially looks for ./build/__main__.py).

I tried referencing the module with relative and absolute paths, but it also throws an error:

$ python3 -m .venv/lib/python3.13/site-packages/build --wheel
/Users/jathu/executorch/.venv/bin/python3: Relative module names not supported

$ python3 -m /Users/jathu/executorch/.venv/lib/python3.13/site-packages/build --wheel
/Users/jathu/executorch/.venv/bin/python3: Error while finding module specification for '/Users/jathu/executorch/.venv/lib/python3.13/site-packages/build' (ModuleNotFoundError: No module named '/Users/jathu/executorch/')

@larryliu0820
Copy link
Contributor

So this only happens when you run python3 -m build --wheel inside executorch/, right?

@larryliu0820
Copy link
Contributor

Ah ok I just ran these commands locally and now I understand the issue. Yeah it seems there's no way around but rename build/.

@mergennachin
Copy link
Contributor

However, the bigger impact would be to update our docs that reference build scripts under that folder. This will also make any external tutorials stale since those scripts will not exist. Not a huge problem, but is it worth the effort to reduce wheel building by 1-2 steps?

That's a general problem. But we have a way to solve today by version. We have two versions of the doc.

https://pytorch.org/executorch/main/ - that is mirror of the main branch
https://pytorch.org/executorch/0.5/ - that is mirror of the release branch

Depending on which versions of the code that they're, users will know which version of the doc to read.

@mergennachin
Copy link
Contributor

We have to capture all explicit usage of build/, but there might be also some implicit references. Not impossible, but would require a back and forth on updating, then letting the CI provide some feedback.

Are there that many?

@mergennachin
Copy link
Contributor

In terms of priority, it seems like a nice-to-have but not critical

Maybe a question for you instead, what does this enable moving forward

@larryliu0820
Copy link
Contributor

I think on a high level it's preferred to not have a build/ in project root.

  • Generally developers expect build/ to host temp files.
  • Dev tools making assumptions that build/ contain build files:
    • vs code cmake plugin will automatically write CMake files into build/
    • github website doesn't index any filename inside build/

@mergennachin
Copy link
Contributor

Okay, then let's do this rather early than later.

@jathu jathu added the actionable Items in the backlog waiting for an appropriate impl/fix label Mar 12, 2025
@github-project-automation github-project-automation bot moved this to To triage in ExecuTorch DevX Mar 12, 2025
@jathu jathu added this to the 0.7.0 milestone Mar 12, 2025
jathu added a commit that referenced this issue Mar 13, 2025
### Summary

A series of diffs as a part of
#9117. Move it from
`./build` → `./scripts/build`

### Test plan

CI

cc @larryliu0820 @lucylq
@jathu jathu self-assigned this Mar 15, 2025
jathu pushed a commit to jathu/executorch that referenced this issue Mar 17, 2025
Summary: As part of pytorch#9117, move the constraint targets out of `build/constraints`. Open to suggestions on a different destination.

Differential Revision: D71267118
jathu added a commit that referenced this issue Mar 17, 2025
### Summary
A series of diffs as a part of
#9117.

### Test plan

CI

cc @larryliu0820 @lucylq
jathu added a commit that referenced this issue Mar 17, 2025
### Summary
A series of diffs as a part of
#9117.

### Test plan

CI

cc @larryliu0820 @lucylq
jathu added a commit that referenced this issue Mar 17, 2025
)

### Summary
A series of diffs as a part of
#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
jathu added a commit that referenced this issue Mar 19, 2025
…9383)

### Summary
A series of diffs as a part of
#9117.

### Test plan

CI
jathu added a commit that referenced this issue Mar 20, 2025
### Summary
A series of diffs as a part of
#9117. These are independent
files, that can be safely moved without dependencies.

### Test plan

CI
jathu added a commit that referenced this issue Mar 20, 2025
### Summary
A series of diffs as a part of
#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
jathu added a commit that referenced this issue Mar 20, 2025
### Summary
A series of diffs as a part of
#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
@mergennachin mergennachin moved this from To triage to Ready in ExecuTorch DevX Mar 20, 2025
oscarandersson8218 pushed a commit to oscarandersson8218/executorch that referenced this issue Mar 21, 2025
### Summary

A series of diffs as a part of
pytorch#9117.

### Test plan

CI

```
$ rm -rf pip-out && python setup.py bdist_wheel
```


cc @larryliu0820 @lucylq
oscarandersson8218 pushed a commit to oscarandersson8218/executorch that referenced this issue Mar 21, 2025
oscarandersson8218 pushed a commit to oscarandersson8218/executorch that referenced this issue Mar 21, 2025
### Summary
A series of diffs as a part of
pytorch#9117. These are independent
files, that can be safely moved without dependencies.

### Test plan

CI
oscarandersson8218 pushed a commit to oscarandersson8218/executorch that referenced this issue Mar 21, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
oscarandersson8218 pushed a commit to oscarandersson8218/executorch that referenced this issue Mar 21, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
jathu added a commit that referenced this issue Mar 24, 2025
### Summary
The final diff as part of
#9117. This is the big one
that affects users — we finally move the core build scripts into
`scripts/`

### Test plan

CI

cc @larryliu0820 @lucylq
@jathu jathu closed this as completed Mar 24, 2025
@github-project-automation github-project-automation bot moved this from Ready to Done in ExecuTorch DevX Mar 24, 2025
@mergennachin
Copy link
Contributor

@jathu Looks like you're done? Great job. One last request -- could you update the top level README file and remove the build

https://github.com/pytorch/executorch/blob/main/README.md

mergennachin pushed a commit that referenced this issue Mar 24, 2025
### Summary
TSIA after #9117.

### Test plan
N/A
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI

cc @larryliu0820 @lucylq
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI

cc @larryliu0820 @lucylq
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary

A series of diffs as a part of
pytorch#9117.

### Test plan

CI

```
$ rm -rf pip-out && python setup.py bdist_wheel
```


cc @larryliu0820 @lucylq
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary
A series of diffs as a part of
pytorch#9117. These are independent
files, that can be safely moved without dependencies.

### Test plan

CI
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
DannyYuyang-quic pushed a commit to CodeLinaro/executorch that referenced this issue Apr 2, 2025
### Summary
A series of diffs as a part of
pytorch#9117.

### Test plan

CI


cc @larryliu0820 @lucylq
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Items in the backlog waiting for an appropriate impl/fix module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch module: ci Issues related to continuous integration
Projects
Status: Done
Development

No branches or pull requests

4 participants