search

adriangb / di

Pythonic dependency injection
https://www.adriangb.com/di/
MIT License
301 stars 13 forks source link

perf: use graphlib2 as dependency resolution backend #40

Closed adriangb closed 2 years ago

adriangb commented 2 years ago

There are unfortunately a lot of changes tangled up in here that come from changing the dependency resolution backend from the custom existing one to basically the graphlib API. So before merging this, the following questions need to be answered:

  1. Is graphlib2 better than graphlib for this application? It is clear that it is asymptotically faster, but it may be slower for very small inputs (benchmarks). But graphlib has no easy way to be deep-copied. Maybe we should just tweak the current implementation?
  2. If we do use graphlib2, is it worth using the node id API over the plain hashable API? Pros are it may be faster (need benchmarks to confirm). Cons are that it is not interoperable with the stdlib graphlib implementation.
  3. This change gets rid of subgraph pruning in favor of simply executing no-ops. This is conceptually murkier since it leaves a lot of open questions like what happens in A -> B -> C if A and C are cached but B is not (previously none would get executed, which is the right thing IMO, now B would get executed), but these are mostly edge-cases that can be resolved easily by users. Pruning subgraphs on every execution is very expensive, even if it is done in Rust extension code. I guess we need to be okay w/ this tradeoff.
netlify[bot] commented 2 years ago

❌ Deploy Preview for xenodochial-agnesi-66605a failed.

🔨 Explore the source changes: c786ca1dc2721dc248fb5355b1855d0574acbe93

🔍 Inspect the deploy log: https://app.netlify.com/sites/xenodochial-agnesi-66605a/deploys/61a73ad2188ef000071706f5

codecov-commenter commented 2 years ago

Codecov Report

Merging #40 (ce7c306) into main (6044522) will decrease coverage by 0.36%. The diff coverage is 92.71%.

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   98.36%   97.99%   -0.37%     
==========================================
  Files          61       61              
  Lines        2014     2042      +28     
  Branches      215      303      +88     
==========================================
+ Hits         1981     2001      +20     
- Misses         23       30       +7     
- Partials       10       11       +1
Impacted Files Coverage Δ
di/_utils/state.py 100.00% <ø> (ø)
di/executors.py 76.56% <61.53%> (-12.92%) :arrow_down:
di/_utils/execution_planning.py 100.00% <100.00%> (ø)
di/_utils/task.py 100.00% <100.00%> (ø)
di/api/executor.py 100.00% <100.00%> (ø)
di/container.py 99.59% <100.00%> (+0.03%) :arrow_up:
tests/test_executors.py 100.00% <100.00%> (+2.32%) :arrow_up:
docs/src/textual/src.py 95.45% <0.00%> (-4.55%) :arrow_down:
docs/src/starlette/src.py 100.00% <0.00%> (ø)
di/dependant.py 91.79% <0.00%> (+0.74%) :arrow_up: