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

[BREAKING] Change module imports to private scope #515

Merged
merged 15 commits into from
Dec 18, 2024

Conversation

zghannam
Copy link
Contributor

@zghannam zghannam commented Dec 10, 2024

Refactored the imports for first and third-party imports for issue #510, but not in-package imports yet. Can absolutely change anything if needed. Didn't break anything as far as I can tell (still passing all provided test cases) and was formatted with Black.

@zghannam zghannam changed the title Fix for "Use imports into private scope." #510 Possible fix for "Use imports into private scope." Dec 10, 2024
@sdatkinson
Copy link
Owner

Cool! Yes, this is in the right direction so I'll approve.

My eyes caught a few stragglers like numpy as np.

Since there's so much line diff already, I don't mind merging this PR and taking another PR for more so that it's easier to process piece by piece for me.

@sdatkinson
Copy link
Owner

sdatkinson commented Dec 10, 2024

Ah, actually one other thing we'll want to do is bump the minor version to 0.12.0 since this is breaking. That needs to happen in nam/_version.py and in the docs conf.py.

@zghannam
Copy link
Contributor Author

Yeah sorry about the larger PR. I left the "import numpy as np" and other similar imports since that was actually there already and I didn't know if I should be changing any existing names.

So just to make sure whats left: fix some of the existing import names like "import numpy as np", change the version in the mentioned files, and finish up adding names for imports within the package like "from .lightning_module import LightningModule as _LightningModule"?

@sdatkinson
Copy link
Owner

@zghannam yep! The version bit would be enough for me to merge the PR but all of that and I think it's sufficient to resolve the Issue 👍🏻

@sdatkinson sdatkinson changed the title Possible fix for "Use imports into private scope." [BREAKING] Change module imports to private scope Dec 17, 2024
Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

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

Thanks for all this work; it's quite a diff!

Just a couple things with "[C]" that I'd like to see addressed before I can approve. Nits, feel free to address optionally; I'd be happy to merge either way.

.vscode/settings.json Outdated Show resolved Hide resolved
nam/_version.py Outdated Show resolved Hide resolved
nam/cli.py Outdated Show resolved Hide resolved
nam/cli.py Outdated Show resolved Hide resolved
nam/models/conv_net.py Outdated Show resolved Hide resolved
nam/train/lightning_module.py Outdated Show resolved Hide resolved
@zghannam
Copy link
Contributor Author

Sorry about some of those, don't know how I missed them. Merged with the main branch and those changes should be resolved now

@zghannam zghannam requested a review from sdatkinson December 17, 2024 17:14
Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@sdatkinson sdatkinson merged commit 4fb6408 into sdatkinson:main Dec 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants