[Contrib] Added default non-verbose to download_testdata(), pass to download() #8533
Conversation
Lunderberg
requested review from
comaniac,
jroesch,
junrushao,
tqchen,
yzhliu and
zhiics
as code owners
python/tvm/contrib/download.py
Outdated
| verbose: int, optional | ||
| Verbose level | ||
| verbose: bool, optional | ||
| If status messages should be printed. |
There was a problem hiding this comment.
| If status messages should be printed. | |
| If status messages should be printed. Defaults to false. |
There was a problem hiding this comment.
I like it, and I would add it if not for the change/removal of verbose following your suggestion to use logging.
python/tvm/contrib/download.py
Outdated
| print("exist file got corrupted, downloading %s file freshly..." % path) | ||
| download(url, path, True, False) | ||
| if verbose: | ||
| print(f"Existing file {path} has incorrect size, downloading fresh copy") |
There was a problem hiding this comment.
Is the use of print() intentional? as opposed to using the use of logging module, similar to other parts of our code base?
One advantage of logging is that it can set various log levels, removing the need for the blocks like if verbose: print("message"), and instead, we could use verbose to set log level into e.g. DEBUG in this module.
What do you think?
There was a problem hiding this comment.
No particular reason for me, other than it being the way the file was previously written. Looking through the history, there aren't any commits that suggest a need for print statements, so I agree that logging is the better route to go.
With logging, I think that we can drop the verbose flag altogether. The user can still specify whether they want the verbose output by setting that logger's log level, but it doesn't need to be part of the main function call. For the information in these print statuses, I think they should be at INFO level, except for the download progress bar which can be at DEBUG level.
There was a problem hiding this comment.
Yeah, I agree with the log levels you proposed. Thanks!
There was a problem hiding this comment.
No problem! I ended up using WARNING level a couple of times as well, for the corrupted file and error-retry paths.
python/tvm/contrib/download.py
Outdated
| else: | ||
| TEST_DATA_ROOT_PATH = Path(Path("~").expanduser(), ".tvm_test_data") | ||
| TEST_DATA_ROOT_PATH.mkdir(parents=True, exist_ok=True) | ||
|
|
||
|
|
||
| def download_testdata(url, relpath, module=None, overwrite=False): | ||
| def download_testdata(url, relpath, module=None, overwrite=False, verbose=False): |
There was a problem hiding this comment.
Can you also add overwrite to the docstring?
There was a problem hiding this comment.
Good catch, and can do.
…ownload(). Minor cleanup as well, while in the file - Using tempfile.TemporaryDirectory instead of explicit cleanup. - Pass through verbose/retries arguments if replacing a corrupted copy.
Initial implementation using tempfile.TemporaryDirectory assumed that the tempdir and output location were on the same drive, and could be renamed. This update falls back to copying from the temporary directory, in case the tempdir is on a different drive.
Lunderberg
force-pushed
the
download_testdata_nonverbose
branch
from
36b1479 to
44fa935
Compare
…ownload() (apache#8533) * [Contrib] Added default non-verbose to download_testdata(), pass to download(). Minor cleanup as well, while in the file - Using tempfile.TemporaryDirectory instead of explicit cleanup. - Pass through verbose/retries arguments if replacing a corrupted copy. * [Contrib] Switched download.py from print statements to logging * [Contrib] Added shutil.copy2 fallback after downloading file. Initial implementation using tempfile.TemporaryDirectory assumed that the tempdir and output location were on the same drive, and could be renamed. This update falls back to copying from the temporary directory, in case the tempdir is on a different drive. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
…ownload() (apache#8533) * [Contrib] Added default non-verbose to download_testdata(), pass to download(). Minor cleanup as well, while in the file - Using tempfile.TemporaryDirectory instead of explicit cleanup. - Pass through verbose/retries arguments if replacing a corrupted copy. * [Contrib] Switched download.py from print statements to logging * [Contrib] Added shutil.copy2 fallback after downloading file. Initial implementation using tempfile.TemporaryDirectory assumed that the tempdir and output location were on the same drive, and could be renamed. This update falls back to copying from the temporary directory, in case the tempdir is on a different drive. Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
Intent is to make this utility easier to use in one-off debugging scripts, where extra print statements may make it harder to track down bugs. Did a small bit of cleanup as well, while I was in the file.
Use tempfile.TemporaryDirectory instead of explicit cleanup.
Pass through verbose/retries arguments if replacing a corrupted copy.