Improve bom generation #51

kdumontnu · 2023-10-20T22:47:47Z

Depends on #50

Iterate schematic files in parallel instead of sequentially (eg. waiting for each one to return before parsing and moving to the next)
Add ACCEPT environment variable to regenerate the BOM output
Sort the BOM output both in the example code and in test code to make it repeatable
Add error condition when fetching design data

…files are generated

examples/generate_bom/generate_bom.py

jtran · 2023-10-27T21:34:22Z

allspice/utils/bom_generation.py

+    # Create a retry value for each file so that we can traverse them
+    # in parallel for improved performance


This comment is a little misleading. This code doesn't do anything in parallel. (Maybe round-robin is more accurate?) I would also use a pretty different approach. We basically want to reimplement the JS client code or something even smarter. But I don't know if we care. So I guess this is fine.

Hmmm, maybe I should look at our rate-limiting a little closer. My goal here was to limit any requests by this function to 4 per second. I didn’t implement something with promises/threads because I figured that could still potentially fire off dozens of requests simultaneously, and rate-limit us, even if we waited between requests for each endpoint.

Our rate limiting session should already limit requests to X per second. If it isn't limiting enough, then we should probably adjust the max_calls and period arguments. The point of having a separate rate limiter is to pull out that concern so that rate limiting logic isn't sprinkled all over the codebase.

py-allspice/allspice/ratelimiter.py

Lines 5 to 16 in 7fa7692

class RateLimitedSession(requests.Session):

"""

A requests.Session that is rate limited.

:param max_calls: Maximum number of calls per period

:param period: Time period in seconds

Example:

session = RateLimitedSession(max_calls=10, period=1)

session.get("https://example.com") # Will be rate limited

"""

If you're interested, I can also post my pseudocode first draft of something that's more similar to the JS client code. It doesn't use promises or threads; that would be one of the improvements we could do.

yeah, if you could share that it would be great

McRaeAlex · 2023-10-30T20:44:54Z

allspice/utils/bom_generation.py

+
+    # Create a retry value for each file so that we can traverse them
+    # in parallel for improved performance
+    retry_counter = [60]*len(schdoc_files_in_repo)


retry_counters

McRaeAlex · 2023-10-30T20:52:07Z

allspice/utils/core.py

@@ -17,16 +17,13 @@ def get_all_pcb_components(
    """

    retry_count = 0
-    while True:
+    while retry_count < 60:


We can change this to a for _ in range(60)

McRaeAlex · 2023-10-30T20:55:15Z

tests/test_utils.py

 from allspice.utils.netlist_generation import generate_netlist

 test_repo = "repo_" + uuid.uuid4().hex[:8]

+accept_outputs = bool(os.getenv('ACCEPT'))


If we want to be consistent across repos this should be UPDATE_EXPECTED

McRaeAlex · 2023-10-30T21:11:30Z

tests/test_utils.py

@@ -51,6 +54,13 @@ def _setup_for_generation(instance, test_name):
    )


+def _bom_entry_sort_key(x: BomEntry) -> str:


I think we should wrap the bom writing code and use it in the tests. Should avoid the duplication between the test and generate_bom.py file

shrik450 · 2024-07-01T14:14:54Z

@kdumontnu Since this PR is considerably out of date now, I was thinking of creating issues for all the improvements here and closing it. Are you ok with that?

kdumontnu · 2024-07-01T14:31:31Z

@kdumontnu Since this PR is considerably out of date now, I was thinking of creating issues for all the improvements here and closing it. Are you ok with that?

Yeah, thanks 👍

kdumontnu requested a review from McRaeAlex October 20, 2023 22:47

kdumontnu force-pushed the kd/pcb_netlist branch from a51fc9c to ac571a4 Compare October 24, 2023 15:08

Base automatically changed from kd/pcb_netlist to main October 26, 2023 22:07

kdumontnu added 2 commits October 26, 2023 18:18

Improve BOM generation perf by iterating files in parallel

c610e1f

Sort BOM generated output

40f9bce

kdumontnu force-pushed the kd/improve_bom_perf branch from c168905 to f6e1f8c Compare October 26, 2023 22:18

kdumontnu added 3 commits October 26, 2023 18:18

Make BOM generation test repeatable

ba9525c

Enforce newline ending

f6e1f8c

Move sleep counter out of loop so that we don't blast the servers if …

aeaa67f

…files are generated

jtran reviewed Oct 27, 2023

View reviewed changes

examples/generate_bom/generate_bom.py Outdated Show resolved Hide resolved

Make sort_function name more descriptive

88114bc

kdumontnu changed the title ~~Improve bom generation performance~~ Improve bom generation Oct 27, 2023

kdumontnu force-pushed the kd/improve_bom_perf branch from 17bd1d1 to f775695 Compare October 27, 2023 19:28

Add error if file fails to generate

f40211a

kdumontnu force-pushed the kd/improve_bom_perf branch from f775695 to f40211a Compare October 27, 2023 19:39

jtran reviewed Oct 27, 2023

View reviewed changes

McRaeAlex requested changes Oct 30, 2023

View reviewed changes

jtran mentioned this pull request Oct 31, 2023

Improve polling logic #58

Open

kdumontnu closed this Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve bom generation #51

Improve bom generation #51

kdumontnu commented Oct 20, 2023 •

edited

Loading

jtran Oct 27, 2023 •

edited

Loading

kdumontnu Oct 28, 2023

jtran Oct 30, 2023

kdumontnu Oct 30, 2023

jtran Oct 31, 2023

McRaeAlex Oct 30, 2023

McRaeAlex Oct 30, 2023

McRaeAlex Oct 30, 2023

McRaeAlex Oct 30, 2023

shrik450 commented Jul 1, 2024

kdumontnu commented Jul 1, 2024

		# Create a retry value for each file so that we can traverse them
		# in parallel for improved performance

	class RateLimitedSession(requests.Session):
	"""
	A requests.Session that is rate limited.

	:param max_calls: Maximum number of calls per period
	:param period: Time period in seconds

	Example:

	session = RateLimitedSession(max_calls=10, period=1)
	session.get("https://example.com") # Will be rate limited
	"""

		@@ -51,6 +54,13 @@ def _setup_for_generation(instance, test_name):
		)


		def _bom_entry_sort_key(x: BomEntry) -> str:

Improve bom generation #51

Improve bom generation #51

Conversation

kdumontnu commented Oct 20, 2023 • edited Loading

jtran Oct 27, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrik450 commented Jul 1, 2024

kdumontnu commented Jul 1, 2024

kdumontnu commented Oct 20, 2023 •

edited

Loading

jtran Oct 27, 2023 •

edited

Loading