Feat/update example books #423

M3ssman · 2024-04-25T09:09:02Z

Contains some modifications to the example section, altogether with some minor updates.

Targets some current difficulties in the actual implementation:

docker-compose turned into docker compose
needed to reduce process pool size, actual setup always freezes machine
if only one single book missing process starts from scratch again, hence individual checks

If you find it useful, feel to merge 🙂

jbaiter

Thanks a lot!

I'm not sure what you mean by if only one single book missing process starts from scratch again, do you mean the script should only index files that are missing on disk/in the index?

If so, I think the best way would be to add a --only-missing flag that before indexing checks which docs are already in the index and then skips those.

jbaiter · 2024-04-25T09:25:53Z

example/README.md

    - ~16GiB of free storage
-2. Launch the containers: `docker-compose up -d`
+1. Clear local data dir if exists: `rm -vr data`


I don't think this is neccessary and will cause unneccessary traffic.

Your absolutely right! I just noticed this can do real harm. If the directory has been deleted and the containers create them from scratch it is going ill.

Concerning if only one single book missing ... : The current flow is like this: one book is missing, then tar requests starts again. I tried the example in a place with slow net speed, lots of connection errs and required several tries.
Before altering this I wasn't able to get just the books in place.

example/README.md

jbaiter · 2024-04-25T09:26:48Z

example/README.md

-2. Launch the containers: `docker-compose up -d`
+1. Clear local data dir if exists: `rm -vr data`
+2. Launch containers: `docker compose up -d`
+    - Check permissions for subdirectory `./data` since there goes the load


Suggested change

- Check permissions for subdirectory `./data` since there goes the load

- Make sure you have write permissions for the subdirectory `./data` since this is where the data will be downloaded to

jbaiter · 2024-04-25T09:27:28Z

example/README.md

I'd prefer a --books-only flag to the ingest.py script instead of creating a second one that shares much of the code with the existing script.

jbaiter · 2024-04-25T09:27:58Z

example/ingest_books_only.py

+
+
+# turn on/off diagnostic information
+LOG_LEVEL = 0


please use the logging module from the python stdlib instead of the if LOG_LEVEL > ... + print pattern, makes for much less verbose/nested code.

jbaiter · 2024-04-25T09:30:37Z

example/ingest_books_only.py

+
+
+if __name__ == '__main__':
+    N_CORES = os.cpu_count() // 6 if os.cpu_count() is not None else 1


I'd prefer if this wasn't hardcoded and instead exposed as a --num-workers (just a suggestion) flag.

Is it okay to evaluate those flags with stdlib argparse ?

Btw, what was the rationale behind generate_batches generator?

Or to send a batch of documents in one request to the index?

Btw, what was the rationale behind generate_batches generator?

Increasing parallelism and thus reduce indexing time. By ingesting multiple documents at the same time we can make use of more than one thread for data processing in Solr.

jbaiter · 2024-04-26T12:40:21Z

First of all, thanks for incorporating the feedback :-)
However, that's a pretty large diff, that touches almost every part of the script, for a rather small set of changes.
Can you:

keep the existing style and general structure of the script
keep the changes you need localized to only the places that need them?

And I'd only use the logging API for debugging stuff, the stdout output was intentionally kept rather brief and readable. Spamming a log line for every iteration is kind of excessive.

jbaiter · 2024-04-26T12:21:37Z

example/ingest.py



+# turn on/off diagnostic information


Suggested change

# turn on/off diagnostic information

jbaiter · 2024-04-26T12:30:47Z

example/ingest.py

-            return True
-    return False
+def main_ingest(the_args):
+    _n_worker = the_args.num_workers


you don't have to mark variables as private in a function body, they're private to the function anyway.

jbaiter · 2024-04-26T12:32:20Z

example/ingest.py

-        if not vol_path.exists():
-            return True
-    return False
+def main_ingest(the_args):


this function shouldn't have to know about argparse, just pass the parameters directly, there are not that many 🙃

jbaiter · 2024-04-26T12:34:37Z

example/ingest.py

-        for fut in as_completed(futs):
-            fut.result()
-    print("\n")
+    PARSER = ArgumentParser(description='ingest example data into SOLR')


those aren't really constants either, even if they're in the global scope.

jbaiter · 2024-04-26T13:24:13Z

example/ingest.py

+    LOGGER = Logger(LOGGER_NAME, _calculate_log_level(ARGS.log_level))
+    STDOUT_HANDLER = StreamHandler(sys.stdout)
+    STDOUT_HANDLER.setFormatter(Formatter("%(asctime)s [%(levelname)s] %(message)s", datefmt="%Y-%m-%dT%H:%M:%S"))


I'd probably stick to logging.basicConfig and not even create a dedicated logger, since this is just a small utility script:

Suggested change

LOGGER = Logger(LOGGER_NAME, _calculate_log_level(ARGS.log_level))

STDOUT_HANDLER = StreamHandler(sys.stdout)

STDOUT_HANDLER.setFormatter(Formatter("%(asctime)s [%(levelname)s] %(message)s", datefmt="%Y-%m-%dT%H:%M:%S"))

logging.basicConfig(level=logging.DEBUG if args.debug else logging.WARN)

then you can just call logging.debug in your code.

jbaiter · 2024-04-26T13:24:47Z

example/ingest.py

-            fut.result()
-    print("\n")
+    PARSER = ArgumentParser(description='ingest example data into SOLR')
+    PARSER.add_argument('--log-level', help='like "debug", "info", "error" (default:info)', required=False,


If we only use logging for debug statements, I think we only need a --debug flag to toggle debug logging on.

M3ssman · 2024-04-26T13:56:49Z

Sorry, I didn't realize your latest remarks at first sight!
But anyway, it's all only a proposal. If you can't go with, it's okay and just leave it out.

M3ssman added 2 commits April 25, 2024 10:58

[doc][upd] example docs

6549a08

[app][feat] books only script

3ca60d3

jbaiter requested changes Apr 25, 2024

View reviewed changes

M3ssman added 3 commits April 26, 2024 12:21

[doc][feat] note script args

a928b17

[app][rfct] deprecate attr

849f4b9

[app][rfct] united in args

518f3bb

M3ssman requested a review from jbaiter April 26, 2024 11:14

jbaiter requested changes Apr 26, 2024

View reviewed changes

M3ssman added 2 commits April 26, 2024 15:44

[app][rfct] hopefully incr compliance

ba62136

[app][rfct] slim logging

570b217

[app][rfct] debugging flag

b6f11f0

M3ssman force-pushed the feat/update-example-books branch from 5aca1ab to b6f11f0 Compare April 27, 2024 08:24

[app][rfct] re-order import

984a35f

M3ssman requested a review from jbaiter April 29, 2024 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/update example books #423

Feat/update example books #423

M3ssman commented Apr 25, 2024

jbaiter left a comment

jbaiter Apr 25, 2024

M3ssman Apr 25, 2024

M3ssman Apr 25, 2024

jbaiter Apr 25, 2024

jbaiter Apr 25, 2024

jbaiter Apr 25, 2024

jbaiter Apr 25, 2024

M3ssman Apr 25, 2024

M3ssman Apr 25, 2024

M3ssman Apr 25, 2024

jbaiter Apr 25, 2024 •

edited

Loading

jbaiter commented Apr 26, 2024 •

edited

Loading

jbaiter Apr 26, 2024

jbaiter Apr 26, 2024

jbaiter Apr 26, 2024

jbaiter Apr 26, 2024

jbaiter Apr 26, 2024

jbaiter Apr 26, 2024

M3ssman commented Apr 26, 2024

	- Check permissions for subdirectory `./data` since there goes the load
	- Make sure you have write permissions for the subdirectory `./data` since this is where the data will be downloaded to



		if __name__ == '__main__':
		N_CORES = os.cpu_count() // 6 if os.cpu_count() is not None else 1

Feat/update example books #423

Are you sure you want to change the base?

Feat/update example books #423

Conversation

M3ssman commented Apr 25, 2024

jbaiter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaiter Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

jbaiter commented Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M3ssman commented Apr 26, 2024

jbaiter Apr 25, 2024 •

edited

Loading

jbaiter commented Apr 26, 2024 •

edited

Loading