fix(warehouses/accelerator/electricity_sharepoint): Set an upper limit for max_workers on extract and load.

[martyngigg]

Summary

It was observed that on a machine with 16 cpus (max_workers=20) and some files would fail when extracting content. It is assumed that reading too many files from sharepoint causes issues.

Fixes #156

Summary by CodeRabbit

• Refactor
  • Restructured concurrent processing worker management with centralised default configuration and updated parameter conventions.

[coderabbitai]

Walkthrough

This pull request refactors concurrent worker thread management in the electricity SharePoint data extraction module. It introduces a centralised MAX_WORKERS_DEFAULT constant, renames the max_threads parameter to max_workers for consistency, and implements logic to enforce an effective maximum worker limit by computing the minimum of the default and provided values.

Changes

Cohort / File(s)	Change Summary
Thread pool worker configuration warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py	Added MAX_WORKERS_DEFAULT constant and os import for centralised worker thread capping. Renamed max_threads parameter to max_workers in extract_content_and_read() signature. Updated function logic to calculate effective maximum workers as min(MAX_WORKERS_DEFAULT, max_workers or MAX_WORKERS_DEFAULT) and pass this to ThreadPoolExecutor. Updated docstring to reflect new parameter name and behaviour.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

The changes are straightforward and homogeneous: a single-file modification introducing a constant, a parameter rename, and simple min-based logic for worker capping. The refactoring follows a clear pattern with no branching complexity or multi-file coordination required.

Poem

🐰 A thread pool tamed with care,
MAX_WORKERS now declared,
No more chaos, just a cap,
Data flows without a gap,
Sharepoint's secrets, all laid bare! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix(warehouses/accelerator/electricity_sharepoint): Set an upper limit for max_workers on extract and load." directly describes the primary change in the codebase. The raw summary confirms that the changes introduce a MAX_WORKERS_DEFAULT constant, rename the max_threads parameter to max_workers, and compute an effective upper limit for concurrent processing using min(MAX_WORKERS_DEFAULT, max_workers). The title is specific, concise, and clearly communicates the main objective of the changeset to limit parallelism to prevent failures.
Linked Issues Check	✅ Passed	The linked issue #156 reports missing electricity data in the power_consumption dataset and identifies that files exist on SharePoint but are failing to be ingested. The PR objectives indicate that over-parallelism when reading too many files concurrently from SharePoint is suspected as the cause of failures, and the fix aims to limit concurrency. The code changes implement exactly this objective by adding a MAX_WORKERS_DEFAULT constant and computing an effective upper limit for max_workers using min(MAX_WORKERS_DEFAULT, max_workers or MAX_WORKERS_DEFAULT), preventing ThreadPoolExecutor from using excessive workers. This directly addresses the implied requirement to implement fixes preventing similar failures through concurrency limits.
Out of Scope Changes Check	✅ Passed	All code changes in the pull request are directly related to implementing the concurrency limit fix for issue #156. The changes include adding the MAX_WORKERS_DEFAULT constant, renaming max_threads to max_workers for consistency, updating the extract_content_and_read function signature, modifying ThreadPoolExecutor instantiation to use effective_max_workers, and updating related docstrings and comments. The os import addition supports the implementation requirements. Each modification is necessary and focused on achieving the objective of limiting concurrent processing to prevent SharePoint file extraction failures.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch set-max_workers-electricity-sharepoint-elt

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9267a79 and e3a8680.

📒 Files selected for processing (2)

• warehouses/accelerator/extract_load/electricity_sharepoint/.dlt/config.toml (1 hunks)
• warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py (3 hunks)

🔇 Additional comments (2)

warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py (2)

77-81: Good refactor: parameter rename aligns with ThreadPoolExecutor API.

The rename from max_threads to max_workers correctly aligns with the ThreadPoolExecutor parameter name, improving consistency and clarity.

---

76-124: AI summary contradicts PR objective.

The AI-generated summary states this change "enables increased parallelism", but the PR objectives clearly indicate the purpose is to limit parallelism to fix failures that occurred when max_workers was set to 20 on a 16-CPU machine.

[martyngigg]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py (1)

94-95: Clarify the default and upper limit behaviour in the docstring.

The current wording "Defaults to a maximum of MAX_WORKERS_DEFAULT" is ambiguous. Consider clarifying that max_workers defaults to MAX_WORKERS_DEFAULT (currently 10) and also acts as an upper limit if a larger value is provided.

Apply this diff to improve clarity:

-    :param max_workers (optional): How many threads to use to process the files.
-                                   Defaults to a maximum of MAX_WORKERS_DEFAULT.
+    :param max_workers (optional): How many threads to use to process the files.
+                                   Defaults to MAX_WORKERS_DEFAULT (10), which also serves
+                                   as an upper limit.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9267a79 and e01901e.

📒 Files selected for processing (1)

• warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py (5 hunks)

🔇 Additional comments (5)

warehouses/accelerator/extract_load/electricity_sharepoint/extract_and_load.py (5)

14-14: LGTM!

The os import is required for os.cpu_count() used in the MAX_WORKERS_DEFAULT calculation.

---

41-44: LGTM!

The constant definition addresses the PR objective by capping concurrent workers at 10. The formula sensibly scales with CPU count for machines with fewer processors whilst preventing the over-parallelism that caused SharePoint failures on the 16-CPU machine.

---

85-85: LGTM!

Renaming to max_workers improves consistency with ThreadPoolExecutor's parameter naming.

---

120-120: LGTM! Upper limit logic correctly implemented.

The calculation correctly enforces the upper limit: when max_workers is None, it defaults to MAX_WORKERS_DEFAULT; when provided, it's capped at MAX_WORKERS_DEFAULT. This addresses the past review comment about implementing a true upper limit.

---

121-123: LGTM!

The ThreadPoolExecutor correctly uses effective_max_workers, ensuring the concurrent worker count never exceeds the intended upper limit of 10, which should prevent the SharePoint content extraction failures.