-
-
Notifications
You must be signed in to change notification settings - Fork 323
feat: add print_debug_info
function
#2913
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ianhi - I like the concept here. Just a few suggestions to make it a bit more complete.
""" | ||
from zarr import print_debug_info | ||
|
||
print_debug_info() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make this test a bit more meaningful, let's capture the output and assert that it matches our basic expectations. https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html#accessing-captured-output-from-a-test-function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested two outputs that should always be there. I didn't test every output because at some point that just becomes rewriting the fucntion
src/zarr/__init__.py
Outdated
print(f"python: {platform.python_version()}\n") | ||
|
||
print(f"zarr: {__version__}\n") | ||
for package in ["numcodecs", "numpy", "fsspec", "s3fs", "botocore", "gcsfs"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we include the full list of required and optional dependencies here?
Required dependencies: donfig | numcodecs | numpy | packaging | typing-extensions
Optional dependencies: botocore | cupy-cuda12x | fsspec| numcodecs | rich | s3fs | universal-pathlib | obstore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added all of them. They're in a list because introspecting the package to grab it's own dependencies seemed to add a bit of unnecessary complication
After discussino with @jhamman there is a sequencing issue here. If we change the issue template prior to releasing the function it won't be possible to make an issue! So I think the way forward is to merge the function and then added the issue template chagnes later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I left some minor suggestions to improve the formatting which I think are worth considering, but feel free to take them or leave them.
src/zarr/__init__.py
Outdated
print(f"zarr: {__version__}\n") | ||
print("Required dependencies:") | ||
print_packages(required) | ||
print("Optional dependencies:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print("Optional dependencies:") | |
print("Optional dependencies") | |
print("---------------------") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't think about the "putting this into markdown on GitHub stage" 🤦 . In that case either asterisks or no change is fine by me 👍
Created the
print_debug_info
function from #2907Will render in the issue template like this:
example output:
closes #2907
docs/user-guide/*.rst
changes/