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.

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 imports UI #77

Closed
wants to merge 1 commit into from
Closed

Fix imports UI #77

wants to merge 1 commit into from

Conversation

runame
Copy link
Contributor

@runame runame commented Jan 7, 2025

Update the imports in distributed_shampoo/__init__.py to reflect the latest changes in matrix_functions_types.py.

@runame runame requested a review from tsunghsienlee January 7, 2025 13:26
@runame runame self-assigned this Jan 7, 2025
@facebook-github-bot 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 Jan 7, 2025
@tsunghsienlee
Copy link
Contributor

Just curious that how this was detected because I would like a more robust way to detect this?

@facebook-github-bot
Copy link
Contributor

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

@runame
Copy link
Contributor Author

runame commented Jan 8, 2025

Just curious that how this was detected because I would like a more robust way to detect this?

I only discovered this because I was trying to use the package with

from distributed_shampoo import QRConfig

and got an error.

I was also thinking that we should figure out a more robust way of implementing this. I guess one option is to define __all__ in each file and then import with from xyz import * in distributed_shampoo/__init__.py, but this is still not really automatic. I can try to look into this.

@tsunghsienlee
Copy link
Contributor

Just curious that how this was detected because I would like a more robust way to detect this?

I only discovered this because I was trying to use the package with

from distributed_shampoo import QRConfig

and got an error.

I was also thinking that we should figure out a more robust way of implementing this. I guess one option is to define __all__ in each file and then import with from xyz import * in distributed_shampoo/__init__.py, but this is still not really automatic. I can try to look into this.

If using __all__ is for from xyz import *, we might consider not using that because it is considered as a bad practice.

I am thinking it probably is time to merge the open-sourced folder with Meta internal folder to let both side tests/CIs to test each other if their settings don't conflict with each other.

@facebook-github-bot
Copy link
Contributor

@tsunghsienlee merged this pull request in b601aae.

@runame runame deleted the fix-imports branch January 17, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants