GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add executor to zimscraperlib by benoit74 · Pull Request #211 · openzim/python-scraperlib

Conversation

benoit74 commented Nov 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

This PR enrich the scraperlib with a ScraperExecutor. This class is capable to process tasks in parallel, with a given number of worker threads.

This executor is mainly inspired from sotoki executor, even if we can find other executors in wikihow and in iFixit. wikihow one seems more primitive / ancient, and iFixit is just a pale copy.

For easy review, first commit is simply a copy/paste of sotoki code, and next commit are the adaptations / enhancement for scraperlib

What has been changed compared to sotoki code:

commit ae8edb7:
- automated unit tests obviously
- moved thread_deadline_sec to the executor, should we need to customize it per executor (probably the case, priceless and useful for tests at least)
- added a check if self.no_more: in submit method: allows to stop accepting task even when the executor is just joined and not shutdown
- renamed prefix to executor_name and moved from T- to executor (way more clear in the logs from my experience)
- removed the release_halt method which was misleading / not working (I failed to join and then release_halt and then submit again ... it seems mandatory to join then start (again) then submit)
commit fd5c04a
- changes in join method: in sotoki, the executor wait thread_deadline_sec seconds per thread. This is highly unpredictable when there are many workers (we could wait thread_deadline_sec for first worker, then thread_deadline_sec for second worker, etc ...), and it is a bit weird that last worker in the list has way more time to complete than first one
- new method computes a global deadline for all threads to join, and immediately request all of them to join (should they already be ready to join)
commit 0ce636c
- just a standard log displaying the thread name, useful to use same notation / format in all scrapers even when we want to display the thread name (should be quite common in fact)

This executor will be used right now in mindtouch scraper.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

benoit74 self-assigned this

Nov 5, 2024

benoit74 force-pushed the scraper_executor branch from b87c5b7 to a080294 Compare

November 5, 2024 13:37

codecov Bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ea6505f) to head (0ce636c).

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #211    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           38        39     +1     
  Lines         2221      2327   +106     
  Branches       426       446    +20     
==========================================
+ Hits          2221      2327   +106

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benoit74 added 4 commits

November 5, 2024 14:21


          Import executor from sotoki

711deaf


          Adapt executor to zimscraperlib

ae8edb7


          Rework join to wait once for all threads and not once per thread

fd5c04a


          Add standard log format displaying thread name

0ce636c

benoit74 force-pushed the scraper_executor branch from 3e08c87 to 0ce636c Compare

November 5, 2024 14:24

benoit74 marked this pull request as ready for review

November 5, 2024 14:31

benoit74 requested a review from rgaudin

November 5, 2024 14:31

rgaudin approved these changes

Nov 7, 2024

View reviewed changes

rgaudin left a comment

Copy link

Copy Markdown

Member

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.

LGTM but it lacks proper documentation. It's important the expected behavior is clearly documented so the user can make informed choices

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py


		@property
		def exception(self):
		"""Exception raises in any thread, if any"""

rgaudin Nov 6, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	"""Exception raises in any thread, if any"""
	"""Exception raised in any thread, if any"""

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		self._workers: set[threading.Thread] = set()
		self.no_more = False
		self._shutdown = False
		self.exceptions[:] = []

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	self.exceptions[:] = []
	self.exceptions.clear()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		self._shutdown = False
		self.exceptions[:] = []

		for n in range(self.nb_workers):

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

No single-letter variable

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		try:
		func(**kwargs)
		except Exception as exc:
		logger.error(f"Error processing {func} with {kwargs=}")

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

Suggested change

	logger.error(f"Error processing {func} with {kwargs=}")
	logger.error(f"Error processing function {func.__name__} with {kwargs=}")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		# received None from the queue. most likely shuting down
		return

		raises = kwargs.pop("raises") if "raises" in kwargs.keys() else False

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

those things are now part of the submit API and should be documented there (in its docstring)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		logger.error(f"Error processing {func} with {kwargs=}")
		logger.exception(exc)
		if raises: # to cover when raises = False
		self.exceptions.append(exc)

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

Is this intended behavior? Exceptions are swallowed without a trace (when using raise=False)? If so, this must be clearly documented. Not raising but storing the exception is another valid alternative.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		except queue.Empty:
		break

		def join(self):

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

What's the recommended way to await an executor completion then?
With this, the deadline_sec being mandatory, I can only use join when I want to exit but in a scraper, I imagine the scenario being: I collect all my resources and submit them to the executor then I join and once everything has been processed (once join completes), I can continue.

This is the regular meaning of join.

If we want it to act as a properly exit method, then the user has to track progress manually and this should be clearly specified in the documentation of the executor

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/zimscraperlib/executor.py

		while (
		len(alive_threads) > 0 and datetime.datetime.now(tz=datetime.UTC) < deadline
		):
		e = threading.Event()

rgaudin Nov 7, 2024

Copy link

Copy Markdown

Member

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.

single-letter var

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

This was referenced Nov 7, 2024

Enhance image processing and more openzim/mindtouch#63

Merged

Small changes for mindtouch #213

Merged

benoit74 marked this pull request as draft

November 8, 2024 13:06

benoit74 commented Nov 8, 2024

Copy link

Copy Markdown

Collaborator Author

Converting to draft, we are experimenting with joblib in mindtouch scraper for now

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments

Reviewers

rgaudin

rgaudin approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

benoit74

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

benoit74 commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rgaudin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoit74 commented Nov 8, 2024

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benoit74 commented Nov 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.