rapidsai / deployment

RAPIDS Deployment Documentation
https://docs.rapids.ai/deployment/stable/
9 stars 27 forks source link

remove use of star imports in rapids-sagemaker-hpo notebook #364

Closed jameslamb closed 3 months ago

jameslamb commented 3 months ago

Contributes to #333.

Fixes these issues in rapids-sagemaker-hpo/notebook.ipynb found by ruff:

F403 `from helper_functions import *` used; unable to detect undefined names
F405 `os` may be undefined, or defined from star imports
F405 `recommend_instance_type` may be undefined, or defined from star imports
F405 `summarize_choices` may be undefined, or defined from star imports
F405 `validate_dockerfile` may be undefined, or defined from star imports
E712 Avoid equality comparisons to `True`; use `if use_spot_instances_flag:` for truth checks
F405 `summarize_choices` may be undefined, or defined from star imports
F405 `new_job_name_from_config` may be undefined, or defined from star imports
F405 `summarize_choices` may be undefined, or defined from star imports
F405 `summarize_hpo_results` may be undefined, or defined from star imports
F405 `download_best_model` may be undefined, or defined from star import

I agree with ruff here... I think it'd be clearer to not have the star imports. And doing that also improves the likelihood of linters being able to catch issues in the way those functions being imported like from helper_functions import * are called.

review-notebook-app[bot] commented 3 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

jameslamb commented 3 months ago

Converting this to draft. I think if we give it a bit of time for the conda cache used in these CI jobs to expire, they'll pull in the new, fixed Sphinx package and work again (https://github.com/conda-forge/sphinx-feedstock/pull/160).

I'll check back on this tomorrow.

jameslamb commented 3 months ago

Alright looks like the issue with the sphinx conda-forge package has been resolved, and the builds here are passing 🎉

This is ready for review.