Skip to content
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.

Sign up for GitHub

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

Fix setuptools config #39

Closed
wants to merge 4 commits into from
Closed

Fix setuptools config #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb23923
Fix setuptools config
francois-rozet Nov 6, 2024
986ef45
Drop -e in CI
francois-rozet Nov 6, 2024
4b6cb51
Use -e for mypy test
francois-rozet Nov 6, 2024
2ec4511
Update .github/workflows/gpu-tests.yaml
francois-rozet Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
- name: Create venv and install the package.
run: |
uv venv && source .venv/bin/activate
uv pip install -e ".[dev]"
uv pip install ".[dev]"
- name: Run tests.
run: |
source .venv/bin/activate
Expand Down
4 changes: 3 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ Repository = "https://github.com/facebookresearch/optimizers.git"
"Bug Tracker" = "https://github.com/facebookresearch/optimizers/issues"

[tool.setuptools]
packages = ["distributed_shampoo"]
py-modules = [
"matrix_functions",
"matrix_functions_types",
"optimizer_modules",
]

[tool.setuptools.packages.find]
include = ["distributed_shampoo*"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to do this instead, to avoid packaging the tests and examples:

Suggested change
include = ["distributed_shampoo*"]
exclude = ["*tests", "*examples"]

But I'm not sure how this could be related to the failing GPU tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this does not work. First, without include, setuptools will install too much things (build/lib). Second, exclude seems to have no effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Not sure why exclude has no effect though? Do you know how we can exclude *tests and *examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a lot of long lasting issues and confusion regarding exclude ... (e.g. pypa/setuptools#3346, pypa/setuptools#3260). I would say, the easiest is likely to move these test directories outside of the module (they should not be there anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that makes sense to me. Thanks a lot for your fix and help with debugging!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder this might be caused by how the wildcard matching works in setuptools work. Personally I am not super familiar with this but if we really want to verify this, I might setup something super small to verify this.


[tool.usort]
first_party_detection = false
Loading