Fixed issue #12745 #12781

LuckyPigeon · 2018-10-10T12:54:20Z

Description

Filter prompt sign while copying code from the website. Fixed #12745

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:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

sandeep-krishnamurthy

LGTM.
Build failed for unrelated issue. Restarted it. We should check on build docs site to verify before merging.

@aaronmarkham - Can you please take a look? Thanks.

vishaalkapoor · 2018-10-10T23:25:48Z

LGTM!

aaronmarkham · 2018-10-11T00:08:38Z

I checked http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12781/2/tutorials/gluon/mnist.html
And I found that the first code block will still copy the $.

aaronmarkham · 2018-10-11T00:15:16Z

docs/_static/js/copycode.js

+                    lines[i] = lines[i].replace(re, "");
+                    break;
+                }
+            }
            lines[i] = lines[i].replace(/^\s+|\s+$/g, "");


I know you didn't change this line, but it seems to be throwing errors in the preview.

Uncaught ReferenceError: g is not defined at copycode.js:67 at t.emit (clipboard.min.js:7) at e (clipboard.min.js:7) at e (clipboard.min.js:7) at e (clipboard.min.js:7) at e (clipboard.min.js:7) at new e (clipboard.min.js:7) at t.e (clipboard.min.js:7) at HTMLBodyElement.<anonymous> (clipboard.min.js:7) at HTMLBodyElement.<anonymous> (clipboard.min.js:7)

There's also some 403 errors happening which really are 404's. Seems like there might be a bigger issue going on here...

Actually, it is a line in the updated code block... line 67...

var re = new RegExp(LANG_GP[lang], g);

aaronmarkham · 2018-10-11T15:59:57Z

Looks like we're hung up on a flaky test. I opened an issue here: #12797
I'll retrigger CI, but it might just fail again until that test is disabled.

aaronmarkham · 2018-10-11T16:29:14Z

Now tracking #12788 - might need to rebase after this PR gets merged.

LuckyPigeon · 2018-10-14T07:05:25Z

@aaronmarkham I find that #12788 has closed, but I can't find the new PR issue related with it. Does the changes are already Updated?

sandeep-krishnamurthy · 2018-10-15T19:03:29Z

@LuckyPigeon - The regression is reverted and master is Green now. Can you please rebase your branch once?

aaronmarkham · 2018-10-19T23:55:03Z

@LuckyPigeon Let me know if you need any help.

LuckyPigeon · 2018-10-20T15:49:26Z

@aalexandrov This is my first time using git rebase, this shows up after I rebase.

First, rewinding head to replay your work on top of it...
Applying: Fixed issue 12745
Applying: Fix copycode.js error caused by new code block

Is this means rebase successfully?
Did I need to push after I rebase?

aaronmarkham · 2018-10-22T18:33:51Z

@aalexandrov This is my first time using git rebase, this shows up after I rebase.
First, rewinding head to replay your work on top of it...
Applying: Fixed issue 12745
Applying: Fix copycode.js error caused by new code block
Is this means rebase successfully?
Did I need to push after I rebase?

It looks like it worked, but I think you still need to git push now. We should see another commit, and that commit will trigger CI.

LuckyPigeon · 2018-10-23T00:02:50Z

I encounter a problem while I using git push

I have already using git pull and is already up to date, but still show up the same error.

aaronmarkham · 2018-10-23T00:16:37Z

Take a look at https://cwiki.apache.org/confluence/display/MXNET/MXNet+Development+Guide

The section that talks about making a PR has this as a guide:

git remote add upstream https://github.com/apache/incubator-mxnet
git fetch upstream
git rebase upstream/master

After going through the rebase process, you might have to force push with:

git push --force

LuckyPigeon · 2018-10-23T06:31:18Z

@aaronmarkham After I go through those commands, I encounter this error.

aaronmarkham · 2018-10-23T13:33:39Z

You can't push upstream.
Just do 'git push --force'
That will force push to your fork and it will automatically update your pull request.

LuckyPigeon · 2018-10-25T14:17:29Z

@aaronmarkham It's seems I can't do git push --force, after I do the command and it shows this message.

aaronmarkham · 2018-10-29T20:16:21Z

Maybe try the suggested command and then force? I'm not sure how you're setup. If you'd like we can get on slack and troubleshoot in realtime. I'm in California. Let me know a time that works for you (I'm typically available 845am - 615pm).

aaronmarkham · 2018-10-31T19:29:47Z

Preview: http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12781/6/index.html

Unfortunately, what I'm seeing on the install page, is that it is still copying the $.
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12781/6/install/index.html?platform=Linux&language=Python&processor=CPU

LuckyPigeon · 2018-11-05T13:44:26Z

@aaronmarkham I still don't get it. Why the install page still copying the bash prompt. I have test it functionally on my local machine, but when I put it on the web, it couldn't work anymore.

kalyc · 2018-11-13T00:52:54Z

@aaronmarkham could you please elaborate more here?

stu1130 · 2018-11-21T19:34:47Z

@aaronmarkham ping again

aaronmarkham · 2018-11-21T21:31:34Z

There are about a half dozen document.ready functions that trigger, so this could be an issue with the js.
The fact that it is broken in the CI preview means that you can emulate it locally when you build the docs (running make html from the docs folder).

I was able to see the issue on another page, so it isn't limited to the install page:
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12781/6/tutorials/gluon/mnist.html

@LuckyPigeon can you see that it isn't working on the above link? Can you debug it from there to see why your change isn't working?

vandanavk · 2018-11-27T17:49:46Z

@mxnet-label-bot update [pr-awaiting-response, Doc]

LuckyPigeon · 2019-01-14T07:35:40Z

@sandeep-krishnamurthy Recently, I have started my vacation, and I can continue this PR again.

sandeep-krishnamurthy · 2019-01-26T00:52:05Z

@LuckyPigeon - When you update, please let me know, we can take this to completion.

LuckyPigeon · 2019-01-31T07:31:55Z

@sandeep-krishnamurthy Updated!

aaronmarkham

In the preview here: http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12781/3/tutorials/gluon/mnist.html
The $ is still copied: $ pip install requests jupyter

LuckyPigeon · 2019-02-10T15:44:31Z

@aaronmarkham Does the preview link build successfully? Because I build the code locally and shows the right result.
Here's my local doc build:

aaronmarkham · 2019-02-11T16:22:08Z

No, the preview link I gave in my last comment shows that it isn't working. The $ is still copied.

LuckyPigeon · 2019-02-20T07:14:26Z

@aaronmarkham I'm not sure what went wrong while building docs. I've tried built the code of this pull request on ubuntu 1804 and the $ sign can be removed successfully. So that must be some other thing went wrong while building the docs. Can you recommend some people for help solving the problem? Thanks!

aaronmarkham · 2019-02-21T00:08:14Z

Can you share a preview link to where you have it working? Maybe I can see what's different.

LuckyPigeon · 2019-02-21T06:03:06Z

But I build it on my local machine, so I don't have a preview link. I can only share the _build folder after I built the docs successfully.

anirudhacharya · 2019-03-02T21:08:58Z

@LuckyPigeon @aaronmarkham please revisit this PR and see it to completion.

karan6181 · 2019-03-19T00:42:55Z

@LuckyPigeon Can you please address the review comments?

@aaronmarkham Could you please review this PR again?

Thank you!

LuckyPigeon · 2019-03-26T06:59:00Z

@karan6181 Sure, but I not quite understand what went wrong in the code. Because the amazon machine can't filter the prompt with the code, but when I build the docs on the local machine success.

piyushghai · 2019-04-09T00:24:39Z

@LuckyPigeon Were you able to figure out the issue blocking this PR ?

Roshrini · 2019-04-17T15:54:30Z

Thanks for working on this. This will be useful for uers. @aaronmarkham Can you help @LuckyPigeon by any chance?

aaronmarkham · 2019-04-17T16:07:42Z

The preview link doesn't work anymore. I'm not convinced this is fixed. Seems like a larger issue, really.

roywei · 2019-04-30T17:35:26Z

@LuckyPigeon Thanks for the contribution, could you address the comments?

pinaraws · 2019-05-20T16:36:17Z

@LuckyPigeon Thanks for the contribution, could you address the comments?

LuckyPigeon · 2019-05-22T15:15:52Z

@roywei @pinaraws Sure. But the problem seems has nothing to do with the code of copycode.js , I think it's something wrong while building the docs, I have encounter it recently, but I am still not sure which part goes wrong.

piyushghai · 2019-06-07T22:51:10Z

@LuckyPigeon @aaronmarkham What's the path forward on this PR ?

Roshrini · 2019-06-23T20:56:44Z

@aaronmarkham Is there any issue with docs build as @LuckyPigeon Mentioned?

aaronmarkham · 2019-06-24T16:37:59Z

It is known that the website publish process (including python module beautiful soup doing code injections, along with some of the site's javascript, cause or have bugs. These make the DOM and interactions therewith unpredictable. The full production build to recreate such issues is bulky, difficult, as well as resource and time consuming. I fear that without front-end engineering help and resources (access to sufficient build/server allocations) @LuckyPigeon is going to be forever stuck on this PR. I have looked at the issue, and I see it as a rabbit hole of javascript and publishing bugs that need to be fixed.

karan6181 · 2019-07-18T16:53:18Z

@LuckyPigeon Ping for the update? How should we drive this PR to resolution? Thanks!

szha · 2019-07-21T18:47:05Z

docs/_static/js/copycode.js

@@ -1,3 +1,4 @@
+/*Copy code to clipboard*/


Suggested change

/*Copy code to clipboard*/

piyushghai · 2019-08-02T19:43:35Z

@LuckyPigeon Gentle ping for drive this PR forward.

LuckyPigeon · 2019-08-19T01:21:57Z

@Roshrini @aaronmarkham @karan6181 @piyushghai
Sorry for late reply. I have followed the mxnet doc build instruction and build a doc on my local computer before, and sometimes it will encounter a problem that after I update my code and execute $ make html, it can't detect the update, and after executed the command, index.html stays the same as before.
Like @aaronmarkham said before, I think I am going to need front-end engineering help, who develop the mxnet docs for the best, and I am going to trace the doc code and find out where the problem is.

aaronmarkham · 2019-08-26T16:59:53Z

Hi @LuckyPigeon thanks for following up on this. I wouldn't spend any more time on this issue. We're looking at swapping out all of the javascript and front-end so I don't think we'll have this issue any more... or at least maybe this feature can be added safely to the new site.

If you want to check it out, we have a PR here:
#15884

aaronmarkham · 2019-10-24T22:13:45Z

Closing since the new site doesn't have this function/issue.

LuckyPigeon requested a review from szha as a code owner October 10, 2018 12:54

sandeep-krishnamurthy added Doc pr-awaiting-review PR is waiting for code review labels Oct 10, 2018

sandeep-krishnamurthy approved these changes Oct 10, 2018

View reviewed changes

aaronmarkham reviewed Oct 11, 2018

View reviewed changes

LuckyPigeon added 2 commits November 1, 2018 01:24

Fixed issue 12745

fe2f940

Fix copycode.js error caused by new code block

cfcfa1c

marcoabreu removed Doc pr-awaiting-review PR is waiting for code review labels Nov 27, 2018

LuckyPigeon added 2 commits January 31, 2019 12:40

Update copycode.js

57bb823

Merge branch 'master' into fix_issue_12745

786564f

aaronmarkham suggested changes Feb 5, 2019

View reviewed changes

szha reviewed Jul 21, 2019

View reviewed changes

docs/_static/js/copycode.js

@@ -1,3 +1,4 @@

/*Copy code to clipboard*/

Copy link

Member

szha Jul 21, 2019

Choose a reason for hiding this comment

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

Suggested change

/*Copy code to clipboard*/

aaronmarkham closed this Oct 24, 2019

Fixed issue #12745 #12781

Fixed issue #12745 #12781

Conversation

LuckyPigeon commented Oct 10, 2018

Description

Checklist

Essentials

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

vishaalkapoor commented Oct 10, 2018

aaronmarkham commented Oct 11, 2018

aaronmarkham Oct 11, 2018

Choose a reason for hiding this comment

aaronmarkham Oct 11, 2018

Choose a reason for hiding this comment

LuckyPigeon Oct 11, 2018

Choose a reason for hiding this comment

aaronmarkham commented Oct 11, 2018

aaronmarkham commented Oct 11, 2018

LuckyPigeon commented Oct 14, 2018

sandeep-krishnamurthy commented Oct 15, 2018

aaronmarkham commented Oct 19, 2018

LuckyPigeon commented Oct 20, 2018 • edited Loading

aaronmarkham commented Oct 22, 2018

LuckyPigeon commented Oct 23, 2018 • edited Loading

aaronmarkham commented Oct 23, 2018

LuckyPigeon commented Oct 23, 2018

aaronmarkham commented Oct 23, 2018

LuckyPigeon commented Oct 25, 2018 • edited Loading

aaronmarkham commented Oct 29, 2018 • edited Loading

aaronmarkham commented Oct 31, 2018

LuckyPigeon commented Nov 5, 2018

kalyc commented Nov 13, 2018

stu1130 commented Nov 21, 2018

aaronmarkham commented Nov 21, 2018

vandanavk commented Nov 27, 2018 • edited Loading

LuckyPigeon commented Jan 14, 2019

sandeep-krishnamurthy commented Jan 26, 2019

LuckyPigeon commented Jan 31, 2019

aaronmarkham left a comment

Choose a reason for hiding this comment

LuckyPigeon commented Feb 10, 2019

aaronmarkham commented Feb 11, 2019

LuckyPigeon commented Feb 20, 2019 • edited Loading

aaronmarkham commented Feb 21, 2019

LuckyPigeon commented Feb 21, 2019

anirudhacharya commented Mar 2, 2019

karan6181 commented Mar 19, 2019

LuckyPigeon commented Mar 26, 2019

piyushghai commented Apr 9, 2019

Roshrini commented Apr 17, 2019

aaronmarkham commented Apr 17, 2019

roywei commented Apr 30, 2019

pinaraws commented May 20, 2019

LuckyPigeon commented May 22, 2019

piyushghai commented Jun 7, 2019

Roshrini commented Jun 23, 2019

aaronmarkham commented Jun 24, 2019

karan6181 commented Jul 18, 2019

szha Jul 21, 2019

Choose a reason for hiding this comment

piyushghai commented Aug 2, 2019

LuckyPigeon commented Aug 19, 2019

aaronmarkham commented Aug 26, 2019

aaronmarkham commented Oct 24, 2019

LuckyPigeon commented Oct 20, 2018 •

edited

Loading

LuckyPigeon commented Oct 23, 2018 •

edited

Loading

LuckyPigeon commented Oct 25, 2018 •

edited

Loading

aaronmarkham commented Oct 29, 2018 •

edited

Loading

vandanavk commented Nov 27, 2018 •

edited

Loading

LuckyPigeon commented Feb 20, 2019 •

edited

Loading