Skip to content
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

[Bug] Fix missing Predictor API docs on Website #2573

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

gidler
Copy link
Collaborator

@gidler gidler commented Dec 14, 2022

Issue #, if available:
#2571

Description of changes:

The install_all bash call in build_all_docs was removed in this commit because of a package dependency conflict between tox for testing in eda module and dependencies of the d2lbook tool.

Unfortunately install_all call was required to install modules that are used during autogen of docs, resulting in the website API documentation problem described in the issue.

The fix here is to create a new install_all_no_tests call that avoids installing tests including the tox dependency when eda/[tests] is installed. In order to do this, I cleaned up the bash scripts we use to install packages so that it is less pedantic and less code is repeated.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gidler
Copy link
Collaborator Author

gidler commented Dec 14, 2022

Once this PR passes CI and is approved, we need to also merge the same changes into stable

@github-actions
Copy link

Job PR-2573-a78915e is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-2573/a78915e/index.html

Copy link
Collaborator

@shchur shchur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix, Garrett! LGTM

@gidler
Copy link
Collaborator Author

gidler commented Dec 14, 2022

Verified the Predictor API docs are back on CI built version: http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-2573/a78915e/api/autogluon.predictor.html

Copy link
Contributor

@Innixma Innixma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

}

function install_multimodal {
# launch different process for each test to make sure memory is released
python3 -m pip install --upgrade pytest-xdist
python3 -m pip install --upgrade -e multimodal/[tests]
install_local_packages "multimodal/$1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outside of the scope of this PR, but its a bit concerning we are doing special pip installs afterwards in our tests for multimodal... This isn't replicating the user experience. Especially using uncapped versions in the install.

Copy link
Collaborator

@yinweisu yinweisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix and cleaning up the code!

@gidler gidler merged commit 2efd553 into autogluon:master Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants