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

[SPARK-30084][DOCS] Document how to trigger Jekyll build on Python API doc changes #26719

Closed
wants to merge 5 commits into from

Conversation

@nchammas
Copy link
Contributor

nchammas commented Nov 30, 2019

What changes were proposed in this pull request?

This PR adds a note to the docs README showing how to get Jekyll to automatically pick up changes to the Python API docs.

Why are the changes needed?

jekyll serve --watch doesn't watch for changes to the API docs. Without the technique documented in this note, or something equivalent, developers have to manually retrigger a Jekyll build any time they update the Python API docs.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I tested this PR manually by making changes to Python docstrings and confirming that Jekyll automatically picks them up and serves them locally.

.gitignore Outdated
@@ -45,7 +45,7 @@ dev/create-release/*final
dev/create-release/*txt
dev/pr-deps/
dist/
docs/_site
**/docs/_site/

This comment has been minimized.

Copy link
@nchammas

nchammas Nov 30, 2019

Author Contributor

Without this change, git was not ignoring python/docs/_site.

This comment has been minimized.

Copy link
@srowen

srowen Nov 30, 2019

Member

That's fine, or just explicitly specify python/docs/_site (is there another?)

This comment has been minimized.

Copy link
@nchammas

nchammas Nov 30, 2019

Author Contributor

I'm not aware of any others, so I can update it to just mention Python specifically on a separate line.

@nchammas

This comment has been minimized.

Copy link
Contributor Author

nchammas commented Nov 30, 2019

Not sure who to ping for review on this (maybe CODEOWNERS could help?) so... pinging @srowen. 😄

Copy link
Member

srowen left a comment

(The Github suggested reviewers usually does a better job than any manually maintained reference, and I think we avoid declaring individual code 'owners' FWIW)

.gitignore Outdated
@@ -45,7 +45,7 @@ dev/create-release/*final
dev/create-release/*txt
dev/pr-deps/
dist/
docs/_site
**/docs/_site/

This comment has been minimized.

Copy link
@srowen

srowen Nov 30, 2019

Member

That's fine, or just explicitly specify python/docs/_site (is there another?)

@SparkQA

This comment has been minimized.

Copy link

SparkQA commented Nov 30, 2019

Test build #114639 has finished for PR 26719 at commit 76a5513.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@SparkQA

This comment has been minimized.

Copy link

SparkQA commented Nov 30, 2019

Test build #114640 has finished for PR 26719 at commit e4c1301.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
Copy link
Member

HyukjinKwon left a comment

Hm, I checked jekyll/jekyll#233 and seems they allowed symbolic links when safe configuration is disabled (jekyll/jekyll#824). This seems already disabled by default https://jekyllrb.com/docs/configuration/default/ and not explicitly set in spark/docs/_config.yml.


### Automatically Rebuilding API Docs

`jekyll serve --watch` will only watch what's in `docs/`, and it [won't follow symlinks](https://github.com/jekyll/jekyll/issues/233). That means it won't monitor your API docs.

This comment has been minimized.

Copy link
@HyukjinKwon

HyukjinKwon Nov 30, 2019

Member

If the only reason to use entr is this symbolic links problem, I think it's better to either fix the configurations in jekyll to allow or verify if there are other more problems ..

This comment has been minimized.

Copy link
@srowen

srowen Nov 30, 2019

Member

Nevermind last comment, I'm getting cross-wired here. I think it's reasonable to doc for doc developers. It's not a problem with anything but jekyll serve used for testing locally.

This comment has been minimized.

Copy link
@HyukjinKwon

HyukjinKwon Nov 30, 2019

Member

Hm, but given the problem noted here, it seems like just a config issue. Can we just remove the issue link at least if we'll document this? At least the issue seems not the cause.

This comment has been minimized.

Copy link
@srowen

srowen Nov 30, 2019

Member

What's the config solution here? I think the question is what happens to docs/latest? but that link we need. @nchammas can you clarify?

This comment has been minimized.

Copy link
@HyukjinKwon

HyukjinKwon Nov 30, 2019

Member

The issue linked was resolved by a PR in Jeykll side and the fix should be available by default if I didn't misread (#26719 (review))

The PR says its guided by safe configuration in Jeykll but its disabled by default. So I think either this linked issue was not the actual cause or safe configuration of Jekyll enabled somewhere else.

This comment has been minimized.

Copy link
@nchammas

nchammas Nov 30, 2019

Author Contributor

I tried yesterday to get Jekyll to pick up Python API doc changes via symlink, since Jekyll only monitors files in the directory it's running in, and the Python source lives outside of docs/. I couldn't get it to work. I'll try again.

This comment has been minimized.

Copy link
@nchammas

nchammas Nov 30, 2019

Author Contributor

Tried again. It doesn't work. It looks like Jekyll has bounced around many times on supporting symlinks or not, and it varies depending on the context (main site content vs. themes vs. whatever else). I don't claim to understand it all.

What I can assert, though, is that jekyll serve --watch does not follow symlinks under Jekyll 4.0.0. I posted on the Jekyll forum to confirm.

Here's the change I tested:

--- a/docs/_plugins/copy_api_dirs.rb
+++ b/docs/_plugins/copy_api_dirs.rb
@@ -123,11 +123,14 @@ if not (ENV['SKIP_API'] == '1')
     puts "Moving back into docs dir."
     cd("../../docs")
 
-    puts "Making directory api/python"
-    mkdir_p "api/python"
+    system("rm -rf api/python")
+    system("ln -sf " + pwd + "/../python/docs/_build/html ./api/python")
 
-    puts "cp -r ../python/docs/_build/html/. api/python"
-    cp_r("../python/docs/_build/html/.", "api/python")
+    # puts "Making directory api/python"
+    # mkdir_p "api/python"
+
+    # puts "cp -r ../python/docs/_build/html/. api/python"
+    # cp_r("../python/docs/_build/html/.", "api/python")
   end
 
   if not (ENV['SKIP_RDOC'] == '1')

Rebuilding the docs does not trigger a Jekyll update.

Even if it did, we still need a way to trigger make html to rebuild the Python API docs, which Jekyll is currently not going to do as part of --watch.

Maybe I'm misunderstanding how Jekyll works. If someone else knows how to get Jekyll to automatically pick up and serve API doc updates, I'm all ears. Maybe there is a better way to do this via Jekyll hooks? Or by getting Jekyll to reload plugins when files change (which would address the make html issue)?

nchammas added 2 commits Nov 30, 2019
@SparkQA

This comment has been minimized.

Copy link

SparkQA commented Nov 30, 2019

Test build #114672 has finished for PR 26719 at commit 05eb1b3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
Copy link
Member

srowen left a comment

To clarify, did we find the safe config isn't the issue, or doesn't help?

@nchammas

This comment has been minimized.

Copy link
Contributor Author

nchammas commented Dec 2, 2019

safe is disabled by default, and I cannot find anyplace in docs/ where we've set it to something else. My post on the Jekyll forum didn't get any replies, unfortunately.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-30084] Document how to trigger Jekyll build on Python API doc changes [SPARK-30084][DOCS] Document how to trigger Jekyll build on Python API doc changes Dec 4, 2019
@srowen srowen closed this in 29e09a8 Dec 4, 2019
@srowen

This comment has been minimized.

Copy link
Member

srowen commented Dec 4, 2019

Merged to master

@nchammas nchammas deleted the nchammas:SPARK-30084-watch-api-docs branch Dec 4, 2019
@nchammas

This comment has been minimized.

Copy link
Contributor Author

nchammas commented Dec 4, 2019

I was going to email the dev list to confirm that I haven't missed a simpler way to get what I'm shooting for with this PR. I suppose I can still do that and issue a follow-up PR if there really is a simpler way to get jekyll serve --watch to automatically pickup Python docstring changes.

@HyukjinKwon

This comment has been minimized.

Copy link
Member

HyukjinKwon commented Dec 5, 2019

Yeah, that's fine as a followup I think.

attilapiros added a commit to attilapiros/spark that referenced this pull request Dec 6, 2019
…I doc changes

### What changes were proposed in this pull request?

This PR adds a note to the docs README showing how to get Jekyll to automatically pick up changes to the Python API docs.

### Why are the changes needed?

`jekyll serve --watch` doesn't watch for changes to the API docs. Without the technique documented in this note, or something equivalent, developers have to manually retrigger a Jekyll build any time they update the Python API docs.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

I tested this PR manually by making changes to Python docstrings and confirming that Jekyll automatically picks them up and serves them locally.

Closes apache#26719 from nchammas/SPARK-30084-watch-api-docs.

Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.