chore: Optional pillow requirement#135
Conversation
|
Will regen poetry lock tomorrow for CI |
|
Hello @QuentiumYT, thanks for this PR ! :) Greatly appreciate it ! |
|
Hello :) it should be good now. I can't tell the size difference (didn't built it again) but we need the whole build-essential with cmake & gcc etc which is growing the size by a lot ^^' |
| try: | ||
| await convert_screenshot_to_webp(self.screenshot) | ||
| except ImportError: | ||
| print("WebP conversion skipped: Pillow library is not installed.") |
There was a problem hiding this comment.
🎯 suggestion(non-blocking): Instead of basicaly printing the message, maybe it is better to log it using loguru?
There was a problem hiding this comment.
Well loguru is nowhere in scrap.py, I'm not sure how should I pass the logger to this method. Do I need to pass the logger instance through EcoindexScraper?
bases/ecoindex/cli/app.py and components/ecoindex/scraper/helper.py are the only files using the logger, I'm not sure how I you want it to look like ^^
There was a problem hiding this comment.
Hello @vvatelot, any news on this? Let me know :)
There was a problem hiding this comment.
You can simply pass loguru to EcoindexScraper, like in the cli.
Keep it simple. I just prefer to use a logger than directly a print...
There was a problem hiding this comment.
Here you are @vvatelot :) I was just asking how you want me to propose coherent changes
vvatelot
changed the title
QuentiumYT
force-pushed
the
optional-pillow
branch
from
d2de79a to
554ccc6
Compare
vvatelot
left a comment
vvatelot
left a comment
There was a problem hiding this comment.
Seems good to me !
Are you allowed to merge ?
|
I am not allowed to merge but I allow you to merge :D |
|
Could you create a new version for pypi so I can use it officially? Thanks @vvatelot |
2 participants
When installing EcoIndex scrapper in a Docker image, it needs plenty of dependencies. Why? Just because pillow needs to be build from source as it is a required dependency.
Since installing
sudo apt-get install libtiff5-dev libjpeg8-dev libopenjp2-7-dev zlib1g-dev libfreetype6-dev liblcms2-dev libwebp-dev tcl8.6-dev tk8.6-dev python3-tk libharfbuzz-dev libfribidi-dev libxcb1-devtakes a while and bloat the Dockerfile, this is not so interesting.https://pillow.readthedocs.io/en/stable/installation/building-from-source.html
EcoIndex scrapper only uses pillow to generate a webp version if the user asks for it (which is probably less than 1% of all users). I suggest moving webp to an optional dependency.
Users can now install this little extra with
pip install ecoindex-scraper[webp], preventing a long install with compile time. A nice throw is shown for users that needs a webp screenshot and haven't installed the webp extra :)Thanks for your review, I needed it for a project and hopefully you can make a release soon!