-
-
Notifications
You must be signed in to change notification settings - Fork 37
Integrate zimfarm dev setup #1027
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
base: main
Are you sure you want to change the base?
Conversation
62d06b6 to
de7afb9
Compare
8176974 to
fc088ff
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1027 +/- ##
=======================================
Coverage 92.90% 92.90%
=======================================
Files 73 73
Lines 4229 4230 +1
=======================================
+ Hits 3929 3930 +1
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
audiodude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this approach.
audiodude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update the name of this PR to something like "Integrate Zimfarm dev setup"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
I also feel like nobody did ran this from end-to-end since the zimfarm worker resources were not adequate, or do I miss something?
We should really run this from end-to-end to ensure this setup works correctly.
And by end-to-end, I mean / propose following scenario:
- user log in WP1 UI
- user creates a simple selection (one or two WPEN article for instance, this detail is not important)
- user requests this selection to be ZIMed
- ZIM file is correctly created
- WP1 UI displays ZIM location
- user can download the ZIM
I totally understand that @elfkuzco might need help from @audiodude and myself regarding details on how to run this end-to-end test, but we should not close this issue / PR until we are sure that everything works from end-to-end. Otherwise this is mostly just a waste.
Yes I can definitely help with that. I'll patch this PR and try setting up/running the zimfarm locally and confirm that I can create and download ZIMs. |
|
Updated the files with the recent changes:
|
|
Code LGTM, waiting for e2e test from @audiodude (if I get it correctly) to give my formal approval |
|
I made some minor tweaks to the PR, but it's still not working. My Zimfarm is still reporting the following for requests to EDIT: This is after following the directions in the README and updating my local |
|
Hum, this is indeed a problem. To unblock you, please set It is however not the proper way to solve this situation to merge this PR. We will continuously have new offliner definitions arriving, and all of them should be stored in the local Zimfarm DB so that dev can use mostly any mwoffliner version / definition version. I feel like the |
|
This is the command I used: Here are the logs: |
|
Okay I think I know the problem. In the first step in the README, when I initially create the Docker graph, this path doesn't exist: I've encountered this before, but at that point Docker creates that path as a directory. Then, when we run the create_worker script, it can't overwrite the directory with the private key. |
|
Yes. Oddly enough, it happened to me too. Would update the docs to prevent this from happening to anyone else. |
|
Just want to make sure. Is this line in the docker-compose file supposed to map a file to a file, or a directory to a directory? If it's meant to map a file, we should simply do a |
|
It's supposed to map to a file. I will revise the shell script to mv the key to that path. |
|
Okay my tasks are being picked up by the worker now! But they are failing. I see this in "Scraper stderr": I assume it's because the optimization cache URL I'm sending in is EDIT: If so, I understand that this is an issue for the mwoffliner repo, of course. |
|
Can you use one similar to the minio one configured for the uploader? |
The thing is the container can't access localhost. You can use |
|
Or if you want, you can omit the optimization URL from your task. |
|
Okay I definitely think we can skip the S3 cache for dev scraping. After I got rid of that, I got a new error from mwoffliner, which was: This makes sense, since the worker is running inside of the docker compose network, while my WP1 web/api/backend is running on the host machine. In fact, this is the exact reason we need to have a zimfarm in dev anyways, because we've changed the logic for the ZIM creation to use a dynamic URL from WP1 itself rather than a static file list on S3. I think at this point, I'm going to start working on putting the dev backend server into the docker compose graph as well, with all the updates to configuration and README that are required for that. I'd like to use this same PR and then just merge the whole thing once we have a working, consistent dev environment. |
|
I agree with you. |
|
Yes for dev we should skip the S3 cache, we will not gain much besides pain. And this is more an internal detail to mwoffliner operation, not really needed. I like the idea of adding the backend to the docker graph in same PR. This is a great opportunity to nail down this dev setup issues and have a reproducible setup devs can use from e2e. No more excuses for not testing stuff once in a while from e2e. Also a great asset in term of documentation / learning base. I would even suggest to also add web and api to the docker graph. With proper mount point and configuration it should be possible to have hot reload whenever dev changes something in the codebase, at least this is what we achieved to have in zimfarm, zimit-frontend and cms repos, and it is (mostly?) totally transparent in terms of performances. It free the developers from having anything to install on their dev machine besides Docker, and ensures there is no headaches due to bad versions and stuff like that. Quite important for everyone which is not a core maintainer and / or a bit lazy to setup stuff correctly on his machine (which includes myself ^^) |
|
Okay I've got the following in my docker: I've changed the URL for the article list we send to Zimfarm to try and use the WP1 API that's running in docker, so I'm using I understand that this is a network connectivity issue, and I need to use the right domain for the WP1 API. However, the part I don't understand is the network topology for worker/worker-manger/mwoffliner/etc and where the mwoffliner is actually running on the network. What should I put for |
|
Also tried with It's reachable from |
|
Could you push your recent changes with the wp1 api in the docker graph? |
…e credentials config variable for URLs sent to zimfarm
|
@elfkuzco Done. |
|
@benoit74 , is it possible that my IP is blocked for mwoffliner jobs? I keep getting |
|
It is possible, but quite unlikely ; I would first start by updating the mwoffliner version you use by pulling |
|
@audiodude , don't worry about this anymore so we don't duplicate efforts. I will try and create a reproducible workflow and publish by the end of today and push an updated commit. |
|
So, it's working as expected
Here are some of the things that made it problematic:
The issue seems to be from upstream in that the dnscache image isn't connected to the same netowork. I made some small fixes to my local worker and pointed it to use that image and that's how I got the results. Will make a PR upstream for this to get the dnscache connected to the same network as the rest of the containers when in development mode.Here's what I used to test as the If you look at the latest commit, I removed the directive to overwrite the command. Zimfarm is changing fast these days and it's best to not have that directive there since it might be obsolete. Best to rely on the latest image to do the right thing. I set the command to I will make additional PRs here to point hte script to fetch the latest offliner versions from farm.openzim.org instead of the scraper repository itself too. |
|
Awesome, thanks so much for all of your work on this!
I'm not sure I like pull always for starting the entire docker graph, because it means I have to download things like Redis and MariaDB every time I start the server, which makes things slow and wastes resources. Is there a way we can set that option in the docker compose file for just the zimfarm components?
I will wait for your changes and then approve and merge. |
I think this is probably because they aren't pinned to any specific version. Ideally, only zimfarm components should have |
|
Regarding I would recommend to not add this flag in commands but add a very clear message saying that developers should regularly pull latest Docker images from the Zimfarm and other components with |
|
@audiodude , See latest commit made recently now. Everything works as expected. Here's my settings for the ZIMFARM part of the "ZIMFARM": {
"url": "http://zimfarm-api/v2",
"s3_url": "http://localhost:9000/org-kiwix-dev-zims",
"user": "admin",
"password": "admin",
"hook_token": None,
"definition_version": "1.17.2",
"image": "ghcr.io/openzim/mwoffliner:1.17.2"
}Also updated the shell scripts to fetch offliner definitions from Review and let me know if there's any additional thing to fix |
|
I recommend this My goal for this PR is that:
|


Rationale
As part of cleanup in zimfarm API (openzim/zimfarm#1391), requests to create recipes/tasks now require an offliner definition version. This PR sets the version of the offliner definition from env variable and sets up zimfarm containers in a docker-compose graph. Previously, the API used "initial" as the definition versions but as scrapers evolve and arguments change, the definitions change too.
Changes