Skip to content

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to switch pygments.rb, which only support Python 2 and seems inactive for the last few years (https://github.com/tmm1/pygments.rb), to Rouge which is pure Ruby code highlighter that is compatible with Pygments.

I thought it would be pretty difficult to change but thankfully Rouge does a great job as the alternative.

Why are the changes needed?

We're moving to Python 3 and drop Python 2 completely.

Does this PR introduce any user-facing change?

Maybe a little bit of different syntax style but should not have a notable change.

How was this patch tested?

Manually tested the build and checked the documentation.

@HyukjinKwon
Copy link
Member Author

I'm going to closely double check tomorrow again. I just roughly checked.

@HyukjinKwon
Copy link
Member Author


ARG BASE_PIP_PKGS="setuptools wheel virtualenv"
ARG PIP_PKGS="pyopenssl pypandoc numpy pygments sphinx"
ARG PIP_PKGS="pyopenssl pypandoc numpy sphinx"
Copy link
Member Author

@HyukjinKwon HyukjinKwon Nov 14, 2019

Choose a reason for hiding this comment

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

Sphinx still needs pygments but it's in sphinx's dependency. I don't think we should explicitly list since Spark doesn't directly use it (but sphinx uses it).

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @vanzin too

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

If the doc build works, seems fine to me.


You need to have [Ruby](https://www.ruby-lang.org/en/documentation/installation/) and
[Python](https://docs.python.org/2/using/unix.html#getting-and-installing-the-latest-version-of-python)
installed. Also install the following libraries:
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to mention Python 3 is required here? or does it still work with Python 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it still works with Python 2 because it seems not using Python at all. But I guess we should better stick to Python 3 ..

@SparkQA
Copy link

SparkQA commented Nov 14, 2019

Test build #113785 has finished for PR 26521 at commit 2b142b3.

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

@HyukjinKwon
Copy link
Member Author

Right, I double checked and seems fine. should be good to go. I am merging this.

@HyukjinKwon
Copy link
Member Author

Merged to master.

@dongjoon-hyun
Copy link
Member

Sorry for being late. Thank you for finishing this~

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 26, 2019

Hi, @HyukjinKwon . This seems to break our release script because rouge requires Ruby 2.5.0+ and our spark-rm Docker file has Ruby 2.3.

$ ruby -v
ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-darwin18]

$ gem install rogue
ERROR:  Error installing rogue:
	sprockets requires Ruby version >= 2.5.0.

@dongjoon-hyun
Copy link
Member

cc @gatorsmile and @jiangxb1987

@srowen
Copy link
Member

srowen commented Nov 26, 2019

Oh, I think that's a typo. It's rouge not rogue, right?

@jiangxb1987
Copy link
Contributor

oops

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 26, 2019

Oh... It's weird. Although this PR and https://github.com/apache/spark-website/pull/233/files#diff-04c6e90faac2675aa89e2176d2eec7d8R9 uses rogue. Yes. It should be rouge.

@srowen
Copy link
Member

srowen commented Nov 26, 2019

If you haven't already opened a PR, I'll fix that.

dongjoon-hyun added a commit that referenced this pull request Nov 26, 2019
### What changes were proposed in this pull request?

This PR aims to fix a type; `rogue` -> `rouge` .
This is a follow-up of #26521.

### Why are the changes needed?

To support `Python 3`, we upgraded from `pygments` to `rouge`.

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

No. (This is for only document generation.)

### How was this patch tested?

Manually.
```
$ docker build -t test dev/create-release/spark-rm/
...
1 gem installed
Successfully installed rouge-3.13.0
Parsing documentation for rouge-3.13.0
Installing ri documentation for rouge-3.13.0
Done installing documentation for rouge after 4 seconds
1 gem installed
Removing intermediate container 9bd8707d9e84
 ---> a18b2f6b0bb9
...
```

Closes #26686 from dongjoon-hyun/SPARK-28752.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@HyukjinKwon
Copy link
Member Author

Oops ..

attilapiros pushed a commit to attilapiros/spark that referenced this pull request Dec 6, 2019
### What changes were proposed in this pull request?

This PR aims to fix a type; `rogue` -> `rouge` .
This is a follow-up of apache#26521.

### Why are the changes needed?

To support `Python 3`, we upgraded from `pygments` to `rouge`.

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

No. (This is for only document generation.)

### How was this patch tested?

Manually.
```
$ docker build -t test dev/create-release/spark-rm/
...
1 gem installed
Successfully installed rouge-3.13.0
Parsing documentation for rouge-3.13.0
Installing ri documentation for rouge-3.13.0
Done installing documentation for rouge after 4 seconds
1 gem installed
Removing intermediate container 9bd8707d9e84
 ---> a18b2f6b0bb9
...
```

Closes apache#26686 from dongjoon-hyun/SPARK-28752.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
HyukjinKwon added a commit that referenced this pull request Jan 13, 2020
…ekyll properly via Rouge

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

This PR proposes to use Pygment compatible format by Rouge. As of #26521, we use Rouge instead of Pygment wrapper in Ruby.
Rouge claims Pygment compatibility; and we should output as Pygment does.

```ruby
Rouge::Formatters::HTMLPygments.new(formatter)
```

wraps codes with `<div class="highlight"><pre>...` properly.

### Why are the changes needed?

To keep the documentation pretty and not broken.

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

Theoretically, no.

This is rather a regression fix in documentation (that happens only by #26521 in master). See the malformed doc in preview - https://spark.apache.org/docs/3.0.0-preview2/sql-pyspark-pandas-with-arrow.html

### How was this patch tested?

Manually built the doc.

**Before:**
![Screen Shot 2020-01-13 at 10 21 28 AM](https://user-images.githubusercontent.com/6477701/72229159-ba766a80-35ef-11ea-9a5d-9583448e7c1c.png)

**After:**

![Screen Shot 2020-01-13 at 10 26 33 AM](https://user-images.githubusercontent.com/6477701/72229157-b34f5c80-35ef-11ea-8b3a-492e8aa0f82a.png)

Closes #27182 from HyukjinKwon/SPARK-28752-followup.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
@HyukjinKwon HyukjinKwon deleted the SPARK-28752 branch March 3, 2020 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants