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

Add more webdriver options to Map._to_png() #1620

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

WooilJeong
Copy link
Contributor

fix: folium.folium.Map._to_png()의 Firefox path related errors

[Problem]
When the folium.folium.Map._to_png() method is used in an environment where the Firefox web browser is not installed, the following error is output.

[Example]

WebDriverException: Message: 'geckodriver' executable needs to be in PATH.

[Solution]

Added a Boolean argument called chrome to the folium.folium.Map._to_png() method. If this value is False, Firefox webdriver is used as before, and if this value is True, Chrome webdriver is used. Additionally, we used the webdriver_manager package, which automatically installs chromedriver considering the version of the Chrome web browser installed on the client.

[Test]

  • Legacy error-generating logic
import io
import folium
from PIL import Image

m = folium.Map(location = [37.53, 127.054],
               zoom_start = 14)

img_data = m._to_png(5)
img = Image.open(io.BytesIO(img_data))
  • Logic applied to the solution
import io
import folium
from PIL import Image

m = folium.Map(location = [37.53, 127.054],
               zoom_start = 14)

img_data = m._to_png(delay=5,
                     chrome=True)
img = Image.open(io.BytesIO(img_data))

@WooilJeong WooilJeong changed the title fix: folium.folium.Map._to_png()의 Firefox path related errors fix: folium.folium.Map._to_png() - Firefox path related errors Sep 30, 2022
@Conengmo
Copy link
Member

Conengmo commented Oct 7, 2022

Thanks for your submission @WooilJeong! Very clear PR.

Though I agree with the problem description, there are two things I'm critical of in this PR:

  • I wonder if we can do better than a chrome boolean, since it's not intuitive that chrome=False means we'll use Firefox.
  • Using webdriver_manager on the fly is not a good idea I think.

Just to understand why it is the way it currently is: as far as I know the _to_png was never meant to be used by users, but I understand that happens anyway.

A cleaner approach to offer more flexibility in my opinion is to add a webdriver argument, such that a user can pass any webdriver object that works for them. If no webdriver is provided, we default to the current setting.

Alternatively, allow providing a string that corresponds to one of the webdriver modules: 'chrome', 'firefix', 'edge', 'safari', etc.. With the default being 'firefox' to correspond with the current situation.

In either case I think making sure the webdriver works on the system is the responsibility of the user, not folium.

What do you think @WooilJeong?

@Conengmo Conengmo changed the title fix: folium.folium.Map._to_png() - Firefox path related errors Add more webdriver options to Map._to_png() Oct 7, 2022
@WooilJeong
Copy link
Contributor Author

WooilJeong commented Oct 8, 2022

Thank you so much @Conengmo for kindly reviewing my PR and for suggesting a more reasonable way.

I think I overlooked that users can also use other webdriver like safari or edge besides firefox and chrome.

However, among the webdrivers provided by selenium, I only know about Chrome. For example, in the case of chrome, the webdriver path must be specified directly, but in the case of firefox, it seems that the path does not need to be specified. I don't know much about other webdrivers. so why not leave setting up other webdrivers to other contributors who know more about it?

And for chrome webdriver, the version of the chrome web browser installed in the OS and the webdriver version to be run in selenium must match. Also, as mentioned briefly above, selenium settings seem to be slightly different depending on the supported webdriver. Therefore, it seems to be necessary to use both the webdriver path (ex. './chromedriver.exe') together with the string (ex. 'chrome') indicating the type of webdriver as an argument of _to_png().

Based on the advice you gave me and what I mentioned above, I re-edited the logic of _to_png() as follows.

    def _to_png(self, delay=3, webdriver_type='firefox', webdriver_path=None):
        """Export the HTML to byte representation of a PNG image.

        Uses selenium to render the HTML and record a PNG. You may need to
        adjust the `delay` time keyword argument if maps render without data or tiles.

        Examples
        --------
        >>> m._to_png()
        >>> m._to_png(time=10)  # Wait 10 seconds between render and snapshot.

        """
        if self._png_image is None:
            from selenium import webdriver
            if webdriver_type == "chrome":
                options = webdriver.ChromeOptions()
                options.add_argument('--headless')
                driver = webdriver.Chrome(webdriver_path, options=options)
            else:
                options = webdriver.firefox.options.Options()
                options.add_argument('--headless')
                driver = webdriver.Firefox(options=options)
            html = self.get_root().render()
            with temp_html_filepath(html) as fname:
                # We need the tempfile to avoid JS security issues.
                driver.get('file:///{path}'.format(path=fname))
                driver.maximize_window()
                time.sleep(delay)
                png = driver.get_screenshot_as_png()
                driver.quit()
            self._png_image = png
        return self._png_image

And here's an example usage:

import io
import folium
from PIL import Image

m = folium.Map(location = [37.53, 127.054],
               zoom_start = 14)

img_data = m._to_png(delay=5,
                     webdriver_type='chrome',
                     webdriver_path='./chromedriver.exe')
img = Image.open(io.BytesIO(img_data))

@Conengmo
Copy link
Member

Conengmo commented Nov 9, 2022

To get this merged, I opted to simplify this a bit and just expose a driver argument. That enables all use cases somebody might have, while not increasing our code burden.

@Conengmo Conengmo added the ready PR is ready for merging label Nov 9, 2022
@ocefpaf ocefpaf merged commit 7ba08bf into python-visualization:main Nov 9, 2022
@Conengmo
Copy link
Member

Conengmo commented Nov 9, 2022

Thanks for your PR @WooilJeong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready PR is ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants