Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Remove unused files in Website doc #16722

Merged
merged 5 commits into from
Apr 13, 2020
Merged

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Nov 5, 2019

Description

After the revamping of the MXNet website, we no longer need lots of docs from

python/mxnet/ndarray_doc.py
python/mxnet/symbol_doc.py

Only the _build_doc functions described in these 2 docs are used. Hence keeping that and yanking the rest.

Realized this while closing my previous PR - #14243

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Code is well-documented:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • modified: python/mxnet/ndarray_doc.py
  • modified: python/mxnet/symbol_doc.py

@aaronmarkham @sojiadeshina Please confirm

@ChaiBapchya
Copy link
Contributor Author

Turns out the CI is failing coz register.py uses _build_doc
Eg
https://github.com/ChaiBapchya/incubator-mxnet/blob/f1b7b491bb5b1c7c9f7c3fa893af0abdc151cb0e/python/mxnet/symbol/register.py#L29

Does this mean I can remove rest of the contents of the symbol_doc and ndarray_doc files but keep the _build_doc function since it is being used here
OR
Should I remove the function call to _build_doc (so that the CI passes) - I just want to make sure doc building doesn't depend on the 2 files in any way.

@aaronmarkham @sojiadeshina can either of you confirm? Thanks

@ChaiBapchya
Copy link
Contributor Author

I've gone ahead and made the change.
Kept _build_doc and yanked the rest of the functions (doc related) which aren't used anymore.
If the CI passes, would ping folks for review & merge.

@ChaiBapchya
Copy link
Contributor Author

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [centos-cpu, miscellaneous, unix-cpu, centos-gpu, edge, clang, unix-gpu, website, windows-cpu, windows-gpu, sanity]

@leezu
Copy link
Contributor

leezu commented Apr 13, 2020

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [miscellaneous, edge, sanity, clang, website, windows-cpu, centos-cpu, unix-gpu, centos-gpu, unix-cpu, windows-gpu]

@leezu leezu requested a review from marcoabreu as a code owner April 13, 2020 01:33
@leezu leezu removed the request for review from marcoabreu April 13, 2020 01:33
@@ -54,7 +54,7 @@ def _trim_container_id(cid):
return cid[:12]

def __init__(self):
self._docker_client = docker.from_env()
self._docker_client = docker.from_env(timeout=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

@marcoabreu FYI after rebuilding our Unix CPU Jenkins Slave AMI via the scripts in https://github.com/apache/incubator-mxnet-ci/ we intermittently run into docker/docker-py#2266 .

The rootcause is that when a new instance is spawned, booted, Docker agent started and Jenkins immediately requests starting a container, docker may take longer than 60 seconds to finish the startup. In the log, line 294 to 295 you can see that the startup took 70 seconds, triggering the timeout.

The issue is tracked upstream at docker/docker-py#2266 and appears to be a regression in Docker. (Note that we're using a 2 year old Docker version previously, as the AMI hasn't been regenerated.)

Considering we're talking about a socket connecting to localhost, we have no risk of connection issues and do not need to use a timeout.

Added the fix to this PR, as this PR experienced the issue and is ready to be merged.

@leezu leezu merged commit 7a59239 into apache:master Apr 13, 2020
@ChaiBapchya ChaiBapchya deleted the remove_unused_file branch April 13, 2020 16:44
@leezu leezu mentioned this pull request Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants