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

Rename package to instructlab.sdg from instructlab_sdg #14

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

russellb
Copy link
Member

@russellb russellb commented Jun 4, 2024

Part of issue #2

This changes to use Python namespace packages where instructlab is
the namespace used for our libraries.

Signed-off-by: Russell Bryant [email protected]


@russellb russellb requested a review from tiran June 4, 2024 20:24
@russellb
Copy link
Member Author

russellb commented Jun 6, 2024

The instructlab side went in, so this can go in now once I rebase it.

@russellb russellb force-pushed the namespace-package branch 2 times, most recently from b193466 to f5c27f1 Compare June 7, 2024 00:45
@nathan-weinberg nathan-weinberg self-requested a review June 7, 2024 00:51
@russellb russellb force-pushed the namespace-package branch 3 times, most recently from 9fa49d9 to 125f72d Compare June 7, 2024 08:47
@russellb
Copy link
Member Author

russellb commented Jun 7, 2024

I think I'm down to a linter fight between ruff wanting this import to move to its own first party section

+# First Party
+from instructlab.sdg import utils

but if I do that, there's another linter complaining that all of the instructlab imports aren't grouped together

@@ -0,0 +1 @@
__path__ = __import__("pkgutil").extend_path(__path__, __name__)
Copy link
Contributor

@bjhargrave bjhargrave Jun 7, 2024

Choose a reason for hiding this comment

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

Is this __init__.py needed here?

This will allow some portions of a namespace to be legacy portions while others are migrated to PEP 420.

https://peps.python.org/pep-0420/#migrating-from-legacy-namespace-packages

indicates (to me) that it is not necessary in the PEP 420 parts of the namespace (like this and instructlab.schema) to have an instructlab/__init__.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's needed for now until we have reconfigured all tooling and linters to deal with PEP 420 namespace packages.

Copy link
Member

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

Looks good, waiting on gate to be fixed

Part of issue instructlab#2

This changes to use Python namespace packages where `instructlab` is
the namespace used for our libraries.

Signed-off-by: Russell Bryant <[email protected]>
@russellb russellb force-pushed the namespace-package branch from 125f72d to 299381b Compare June 10, 2024 04:25
@russellb
Copy link
Member Author

I fixed the linter issue.

@russellb russellb merged commit 9867dd8 into instructlab:main Jun 10, 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.

6 participants