refactor: packages import spack.package explicitly#30404
refactor: packages import spack.package explicitly#30404tgamblin merged 46 commits intospack:developfrom
spack.package explicitly#30404Conversation
|
Thanks! Two additional things to think about:
|
|
I thought importing of Where was it not getting imported? |
|
It is. The purpose of this PR is to remove that magic, or at least move in a direction in which we can remove that magic in the future. Our magic doesn't work for a lot of tools like autocompletion in Jupyter Notebooks and type checks in mypy. It's also slower than simply using the import directly. EDIT: Another reason is that our magic causes error messages to be off by one line. |
Yes, I agree with all of that, though after a chat with @tgamblin, we were discussing a rename as well, and I have a feeling I should do all of that together. The preference was |
|
Basically I think |
|
Personally I would rename spack.package to something else and rename spack.pkgkit to spack.package. I think we already had this discussion when we first decided on the name spack.pkgkit. |
|
When this goes in we can remove import boilerplate from |
|
Some, but not all yet, there are some things this doesn't cover, but this gets us far enough that it helps a lot identifying what's left. I wouldn't hate renaming package necessarily, but that will touch a lot of places. What would we rename it to, package_class, package_impl, package_internal..? |
|
Ok, this was everything and re-updated on develop less than 10 seconds ago. It now has a conflict. Will probably have to stage this one carefully. Anyway, I ended up moving package to package_base as we discussed, and then pkgkit is now package_defs. I know we discussed making it package, but having the ambiguity between spack.package and the package.py file in every package directory proved somewhat unfortunate. If the decision is to make it package.py, I'll change it one last time to that. |
|
Something bad happened to the github actions here. Going to merge in develop to poke it. |
…CHECK_PACKAGES env var to inform style to check packages as well as core, also basic config for pyright added to pypackage.toml, pyright tests do *NOT* pass, it is much more strict than our mypy config right now and can catch many bugs including missing imports and uninitialized variable names, currently best used on a small set of files, the full repo gives over 90,000 diagnostics with it for now
trws
changed the title
|
@spackbot run pipeline |
|
I had a problem triggering the pipeline. |
tgamblin
changed the title
spack.package explicitly
Explicitly import package utilities in all packages, and corresponding fallout.
This includes:
* rename `spack.package` to `spack.package_base`
* rename `spack.pkgkit` to `spack.package`
* update all packages in builtin, builtin_mock and tutorials to include `from spack.package import *`
* update spack style
* ensure packages include the import
* automatically add the new import and remove any/all imports of `spack` and `spack.pkgkit`
from packages when using `--fix`
* add support for type-checking packages with mypy when SPACK_MYPY_CHECK_PACKAGES
is set in the environment
* fix all type checking errors in packages in spack upstream
* update spack create to include the new imports
* update spack repo to inject the new import, injection persists to allow for a deprecation period
Original message below:
As requested @adamjstewart, update all packages to use pkgkit. I ended up using isort to do this,
so repro is easy:
```console
$ isort -a 'from spack.pkgkit import *' --rm 'spack' ./var/spack/repos/builtin/packages/*/package.py
$ spack style --fix
```
There were several line spacing fixups caused either by space manipulation in isort or by packages
that haven't been touched since we added requirements, but there are no functional changes in here.
* [x] add config to isort to make sure this is maintained going forward
Regression re-introduced in spack#30404
Regression re-introduced in #30404
* fix package-base exception * Update packages/key4hep-stack/common.py Co-authored-by: Valentin Volkl <[email protected]>
Explicitly import package utilities in all packages, and corresponding fallout.
This includes:
* rename `spack.package` to `spack.package_base`
* rename `spack.pkgkit` to `spack.package`
* update all packages in builtin, builtin_mock and tutorials to include `from spack.package import *`
* update spack style
* ensure packages include the import
* automatically add the new import and remove any/all imports of `spack` and `spack.pkgkit`
from packages when using `--fix`
* add support for type-checking packages with mypy when SPACK_MYPY_CHECK_PACKAGES
is set in the environment
* fix all type checking errors in packages in spack upstream
* update spack create to include the new imports
* update spack repo to inject the new import, injection persists to allow for a deprecation period
Original message below:
As requested @adamjstewart, update all packages to use pkgkit. I ended up using isort to do this,
so repro is easy:
```console
$ isort -a 'from spack.pkgkit import *' --rm 'spack' ./var/spack/repos/builtin/packages/*/package.py
$ spack style --fix
```
There were several line spacing fixups caused either by space manipulation in isort or by packages
that haven't been touched since we added requirements, but there are no functional changes in here.
* [x] add config to isort to make sure this is maintained going forward
Regression re-introduced in spack#30404
6 participants
Explicitly import package utilities in all packages, and corresponding fallout. This includes:
spack.packagetospack.package_basespack.pkgkittospack.packagefrom spack.package import *spackandspack.pkgkitfrom packages when using--fixOriginal message below:
As requested @adamjstewart, update all packages to use pkgkit. I ended up using isort to do this, so repro is easy:
There were several line spacing fixups caused either by space manipulation in isort or by packages that haven't been touched since we added requirements, but there are no functional changes in here.