-
Notifications
You must be signed in to change notification settings - Fork 861
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
Update namespace packages for PEP420 compatibility #3228
Update namespace packages for PEP420 compatibility #3228
Conversation
22554c4
to
dfc2ffa
Compare
@@ -106,7 +84,7 @@ def show_versions(): | |||
versions = _get_autogluon_versions() | |||
sorted_keys = sorted(versions.keys(), key=lambda x: x.lower()) | |||
|
|||
maxlen = max(len(x) for x in versions) | |||
maxlen = 0 if len(versions) == 0 else max(len(x) for x in versions) |
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 think previously the CI having error for max() having no argument is a sign that versions is an empty list, and I'm not sure if that's a correct behavior. I wonder does that mean version files are not correctly populated with the installation? Have you verified the installation locally?
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 could not reproduce it across local, container and linux environments.
Another general comment is I'm not quite sure how would this affect pypi installation. Our CI is only testing for local installation and wheel building. We might need to merge this PR and do a test pypi release or wait for the nightly release and double check |
|
||
import autogluon |
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.
How are you importing autogluon?
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.
Same way as before. It works without issues and auto-resolved by the package manager.
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 see, interesting
# We don't include test dependency here | ||
autogluon_extras_dict = { | ||
'autogluon.core': ('all',), | ||
'autogluon.common': (), | ||
'autogluon.features': (), | ||
'autogluon.timeseries': (), | ||
'autogluon.tabular': ('all',), | ||
} | ||
|
||
# This is needed because some module are different in its import name and pip install name | ||
import_name_dict = { | ||
'pillow': 'PIL', | ||
'pytorch-lightning': 'pytorch_lightning', | ||
'scikit-image': 'skimage', | ||
'scikit-learn': 'sklearn', | ||
'smart-open': 'smart_open', | ||
'timm-clean': 'timm', | ||
} | ||
|
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.
Why no longer necessary?
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.
The first dict - autogluon_extras_dict
- the new code filters-out test dependencies via checking extras == 'test'
using regexp.
The second part is not needed because we don't rely on file parsing anymore; packages and versions are retrieved via importlib.metadata.version
and importlib.metadata.distribution
. Thus no mismatch between pip dependency name and package import name.
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.
Got it, thanks!
# Filter-out test dependencies | ||
dependencies = [req for req in dependencies if not bool(re.search('extra.*test', req))] | ||
# keep only package name | ||
dependencies = [re.findall('[a-zA-Z0-9_\\-]+', req)[0].strip() for req in 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.
Have you compared the output of this function on mainline with the output in the PR? Ditto for other changes.
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.
Yes, it fixed a few issues with missing dependencies:
main | new |
---|---|
INSTALLED VERSIONS | INSTALLED VERSIONS |
------------------ | ------------------ |
date : 2023-05-23 | date : 2023-05-23 |
time : 00:49:04.037355 | time : 00:58:15.831350 |
python : 3.9.16.final.0 | python : 3.9.16.final.0 |
OS : Linux | OS : Linux |
OS-release : 5.15.0-1036-aws | OS-release : 5.15.0-1036-aws |
Version : #40~20.04.1-Ubuntu SMP... | Version : #40~20.04.1-Ubuntu SMP ... |
machine : x86_64 | machine : x86_64 |
processor : x86_64 | processor : x86_64 |
num_cores : 8 | num_cores : 8 |
cpu_ram_mb : 31640 | cpu_ram_mb : 31640 |
cuda version : None | cuda version : None |
num_gpus : 0 | num_gpus : 0 |
gpu_ram_mb : [] | gpu_ram_mb : [] |
avail_disk_size_mb : 905325 | avail_disk_size_mb : 898628 |
accelerate : 0.16.0 | accelerate : 0.16.0 |
autogluon : 0.7.1b20230523 | |
autogluon.common : 0.7.1b20230523 | autogluon.common : 0.7.1b20230523 |
autogluon.core : 0.7.1b20230523 | autogluon.core : 0.7.1b20230523 |
autogluon.eda : 0.7.1b20230523 | autogluon.eda : 0.7.1b20230523 |
autogluon.features : 0.7.1b20230523 | autogluon.features : 0.7.1b20230523 |
autogluon.multimodal : 0.7.1b20230523 | autogluon.multimodal : 0.7.1b20230523 |
autogluon.tabular : 0.7.1b20230523 | autogluon.tabular : 0.7.1b20230523 |
autogluon.timeseries : 0.7.1b20230523 | autogluon.timeseries : 0.7.1b20230523 |
boto3 : 1.26.120 | boto3 : 1.26.120 |
catboost : 1.1.1 | catboost : 1.1.1 |
defusedxml : 0.7.1 | defusedxml : 0.7.1 |
evaluate : 0.3.0 | evaluate : 0.3.0 |
fairscale : 0.4.13 | fairscale : 0.4.13 |
fastai : 2.7.12 | fastai : 2.7.12 |
gluonts : 0.12.8 | gluonts : 0.12.8 |
hyperopt : 0.2.7 | hyperopt : 0.2.7 |
imodels : 1.3.17 | |
ipython : None | ipython : 8.12.0 |
ipywidgets : 8.0.6 | ipywidgets : 8.0.6 |
jinja2 : 3.1.2 | jinja2 : 3.1.2 |
joblib : 1.2.0 | joblib : 1.2.0 |
jsonschema : 4.17.3 | jsonschema : 4.17.3 |
lightgbm : 3.3.5 | lightgbm : 3.3.5 |
matplotlib : 3.6.3 | matplotlib : 3.6.3 |
missingno : 0.5.2 | missingno : 0.5.2 |
mlforecast : 0.7.2 | mlforecast : 0.7.2 |
networkx : 2.8.8 | networkx : 2.8.8 |
nlpaug : 1.1.11 | nlpaug : 1.1.11 |
nltk : 3.8.1 | nltk : 3.8.1 |
nptyping : 2.4.1 | nptyping : 2.4.1 |
numpy : 1.23.5 | numpy : 1.23.5 |
omegaconf : 2.2.3 | omegaconf : 2.2.3 |
onnxruntime-gpu : 1.13.1 | |
openmim : None | openmim : 0.3.7 |
pandas : 1.5.3 | pandas : 1.5.3 |
phik : 0.12.3 | phik : 0.12.3 |
PIL : 9.5.0 | Pillow : 9.5.0 |
psutil : 5.9.5 | psutil : 5.9.5 |
pymupdf : None | PyMuPDF : 1.21.1 |
pyod : None | pyod : 1.0.9 |
pytesseract : 0.3.10 | pytesseract : 0.3.10 |
pytorch_lightning : 1.9.5 | pytorch-lightning : 1.9.5 |
pytorch-metric-learning: None | pytorch-metric-learning: 1.7.3 |
ray : 2.3.1 | ray : 2.3.1 |
requests : 2.28.2 | requests : 2.28.2 |
scikit-image : 0.19.3 | |
scikit-learn : 1.1.1 | |
scikit-learn-intelex : None | |
scipy : 1.10.1 | scipy : 1.10.1 |
seaborn : 0.12.2 | seaborn : 0.12.2 |
sentencepiece : 0.1.99 | sentencepiece : 0.1.99 |
seqeval : None | seqeval : 1.2.2 |
setuptools : 65.6.3 | setuptools : 65.6.3 |
shap : 0.41.0 | shap : 0.41.0 |
skimage : 0.19.3 | |
skl2onnx : 1.13 | |
sklearn : 1.1.1 | (see above scikit-learn) |
statsforecast : 1.4.0 | statsforecast : 1.4.0 |
statsmodels : 0.13.5 | statsmodels : 0.13.5 |
suod : None | suod : 0.0.8 |
tensorboard : 2.13.0 | tensorboard : 2.13.0 |
text-unidecode : None | text-unidecode : 1.3 |
timm : 0.6.13 | timm : 0.6.13 |
torch : 1.13.1+cpu | torch : 1.13.1+cpu |
torchmetrics : 0.11.4 | torchmetrics : 0.11.4 |
torchvision : 0.14.1+cpu | torchvision : 0.14.1+cpu |
tqdm : 4.65.0 | tqdm : 4.65.0 |
transformers : 4.26.1 | transformers : 4.26.1 |
ujson : 5.7.0 | ujson : 5.7.0 |
vowpalwabbit : 9.4.0 | |
xgboost : 1.7.5 | xgboost : 1.7.5 |
yellowbrick : 1.5 | yellowbrick : 1.5 |
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.
LGTM, very nice.
eda/setup.py
Outdated
@@ -15,7 +15,7 @@ | |||
########################### | |||
|
|||
version = ag.load_version_file() | |||
version = ag.update_version(version, use_file_if_exists=False, create_file=True) | |||
version = ag.update_version(version) |
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.
Why change?
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.
That was a mistake. Version file should be created only in core
packages=find_packages('src'), | ||
packages=find_namespace_packages('src', include=['autogluon.*']), |
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.
how does the result differ?
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.
From the user side there are no changes. find_namespace_packages
is needed because there's no __init__.py
and PEP asks to use this method instead of find_packages
(it can't detect packages if there no init)
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.
Gotcha, thanks
Job PR-3228-dfc2ffa is done. |
Agreed. I had the same concerns; but I think nightly builds should be the way to check it. |
…pkg_resources improved robustness of the code by using standard ways to retrieve versions instead of relying on parsing of __version__ and __VERSION__ files.
dfc2ffa
to
50f6868
Compare
Rebased onto master |
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.
LGTM!
Job PR-3228-50f6868 is done. |
Verified installation via |
Description of changes:
Building the project generates deprecation warnings and suggests transitioning to the use of implicit namespace packages (refer to native namespace packages and PEP420):
The action requires removing
__init__.py
from the root of the namespace package and switching to usingfind_namespace_packages
instead offind_packages
.Pre-merge checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.