Skip to content

More benchmarks #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seddonym merged 4 commits into from
Jan 18, 2025
Merged

More benchmarks #177

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc9f49b
Assert result within benchmarks directly
Peter554 Jan 18, 2025
6b2c796
Add more benchmarks
Peter554 Jan 18, 2025
9657d65
Add helper to run benchmark local with pendantic vs codspeed
Peter554 Jan 18, 2025
455fed2
Fix bug when building large graph for benchmarks
Peter554 Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 198 additions & 45 deletions tests/benchmarking/test_benchmarking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,23 @@
import grimp


def _run_benchmark(benchmark, fn, *args, **kwargs):
if hasattr(benchmark, "pedantic"):
# Running with pytest-benchmark
return benchmark.pedantic(fn, args=args, kwargs=kwargs, rounds=3)
else:
# Running with codspeed.
return benchmark(fn, *args, **kwargs)


@pytest.fixture(scope="module")
def large_graph():
raw_json = (Path(__file__).parent / "large_graph.json").read_text()
graph_dict = json.loads(raw_json)
graph = ImportGraph()

for importer, importeds in graph_dict.items():
graph.add_module(importer)
Copy link
Collaborator

@seddonym seddonym Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why this changes the result? Adding the import should add any modules as per https://grimp.readthedocs.io/en/stable/usage.html#ImportGraph.add_import

To me this looks like we may have found a bug.

Copy link
Collaborator Author

@Peter554 Peter554 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure I understand why this changed the results and caused the performance slowdown.

One thing that is happening - the graph is now bigger. E.g. consider

{
  "pkg.a": ["pkg.b"],
  "pkg.c": []
}

In the above JSON, the pkg.c module would not have been added before (since it has no imports, and nothing is importing it).

I've reordered the commits, so that the changes caused e.g. in number of modules becomes more obvious now.

Even though I don't fully understand why this happens entirely, I am entirely confident in this change to the benchmarks file. I.e. we certainly should be calling graph.add_module(importer), and the import path that we now find is correct (agrees with pyimports). So I think there's no harm to avoid merging this.

Copy link
Collaborator Author

@Peter554 Peter554 Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I understand it now.

result = large_graph.find_illegal_dependencies_for_layers(
    layers=(
        # mypackage.foo and mypackage.bar do not exist!!!
        "foo",
        "bar",
    ),
    containers=("mypackage",),
)
assert result == set()

The above passes ☝️ So it looks like find_illegal_dependencies_for_layers does not error if the layer modules cannot be found in the graph. Presumably this is by design?

If we look at large graph JSON we have e.g.

"mypackage.domain": [],

This is the only occurence of "mypackage.domain" in that file. So - mypackage.domain is not importing anything, and nothing is importing it - hence the module is not added to the graph.

Copy link
Collaborator Author

@Peter554 Peter554 Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So sadly the test_top_level_large_graph was not doing anything before 😅 Hence why it is now slower - because it actually does some work!

Copy link
Collaborator

@seddonym seddonym Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha ok, makes sense.

The silent failure is by design only to the point of feature parity, but I think it would be worth changing it at some point. Not now though.

for imported in importeds:
graph.add_import(importer=importer, imported=imported)

Expand Down Expand Up @@ -40,13 +50,7 @@ def test_build_django_uncached(benchmark):

In this benchmark, the cache is turned off.
"""
fn = lambda: grimp.build_graph("django", cache_dir=None)
if hasattr(benchmark, "pedantic"):
# Running with pytest-benchmark
benchmark.pedantic(fn, rounds=3)
else:
# Running with codspeed.
benchmark(fn)
_run_benchmark(benchmark, grimp.build_graph, "django", cache_dir=None)


def test_build_django_from_cache(benchmark):
Expand All @@ -57,52 +61,143 @@ def test_build_django_from_cache(benchmark):
"""
# Populate the cache first, before beginning the benchmark.
grimp.build_graph("django")

fn = lambda: grimp.build_graph("django")
if hasattr(benchmark, "pedantic"):
# Running with pytest-benchmark
benchmark.pedantic(fn, rounds=3)
else:
# Running with codspeed.
benchmark(fn)
_run_benchmark(benchmark, grimp.build_graph, "django")


def test_top_level_large_graph(large_graph, benchmark):
benchmark(
lambda: large_graph.find_illegal_dependencies_for_layers(
layers=TOP_LEVEL_LAYERS,
containers=("mypackage",),
)
)


def test_deep_layers_large_graph(large_graph, benchmark):
fn = lambda: large_graph.find_illegal_dependencies_for_layers(layers=DEEP_LAYERS)
if hasattr(benchmark, "pedantic"):
# Running with pytest-benchmark
benchmark.pedantic(fn, rounds=3)
else:
# Running with codspeed.
benchmark(fn)


# Result checks
# -------------
# These tests aren't benchmarks, but they execute the same code as the benchmarks to check the
# behaviour hasn't changed.


def test_top_level_large_graph_result_check(large_graph):
result = large_graph.find_illegal_dependencies_for_layers(
result = _run_benchmark(
benchmark,
large_graph.find_illegal_dependencies_for_layers,
layers=TOP_LEVEL_LAYERS,
containers=("mypackage",),
)

assert result == set()
assert result == {
PackageDependency(
importer="mypackage.domain",
imported="mypackage.application",
routes=frozenset(
{
Route(
heads=frozenset({"mypackage.domain.7960519247.6215972208"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.6928774480.5676105139.3275676604"}
),
),
Route(
heads=frozenset(
{
"mypackage.domain.6928774480.5676105139.1330171288.7588443317.4661445087" # noqa:E501
}
),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.3430454356.1518604543"}
),
),
Route(
heads=frozenset(
{"mypackage.domain.6928774480.5676105139.1262087557.3485088613"}
),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.3430454356.1518604543"}
),
),
Route(
heads=frozenset({"mypackage.domain.2538372545.1186630948"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.8114145747.9320351411"}
),
),
Route(
heads=frozenset(
{
"mypackage.domain.6928774480.1028759677.7960519247.2888779155.7486857426" # noqa:E501
}
),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.3430454356.1518604543"}
),
),
Route(
heads=frozenset({"mypackage.domain.1330171288.2647367251"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.4619328254.6682701798"}
),
),
Route(
heads=frozenset({"mypackage.domain.2538372545.7264406040.9149218450"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.2538372545.8114145747"}
),
),
Route(
heads=frozenset({"mypackage.domain.1330171288.2647367251"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.7582995238.6180716911"}
),
),
Route(
heads=frozenset({"mypackage.domain.1330171288.2647367251"}),
middle=(),
tails=frozenset(
{"mypackage.application.7537183614.3851022211.5970652803"}
),
),
}
),
),
PackageDependency(
importer="mypackage.domain",
imported="mypackage.plugins",
routes=frozenset(
{
Route(
heads=frozenset({"mypackage.domain.8114145747.6690893472"}),
middle=(),
tails=frozenset(
{"mypackage.plugins.5634303718.6180716911.1810840010.7887344963"}
),
)
}
),
),
PackageDependency(
importer="mypackage.application",
imported="mypackage.plugins",
routes=frozenset(
{
Route(
heads=frozenset(
{
"mypackage.application.7537183614.2538372545.1153384736.6297289996",
"mypackage.application.7537183614.2538372545.1153384736.6404547812.6297289996", # noqa:E501
}
),
middle=("mypackage.6398020133.9075581450.6529869526.6297289996",),
tails=frozenset(
{
"mypackage.plugins.5634303718.6180716911.7582995238.1039461003.2943193489", # noqa:E501
"mypackage.plugins.5634303718.6180716911.7582995238.1039461003.6322703811", # noqa:E501
}
),
)
}
),
),
}


def test_deep_layers_large_graph_result_check(large_graph):
result = large_graph.find_illegal_dependencies_for_layers(layers=DEEP_LAYERS)
def test_deep_layers_large_graph(large_graph, benchmark):
result = _run_benchmark(
benchmark, large_graph.find_illegal_dependencies_for_layers, layers=DEEP_LAYERS
)
assert result == {
PackageDependency(
importer=f"{DEEP_PACKAGE}.application.3242334296.2454157946",
Expand Down Expand Up @@ -239,3 +334,61 @@ def test_deep_layers_large_graph_result_check(large_graph):
),
),
}


def test_find_descendants(large_graph, benchmark):
Copy link
Collaborator

@seddonym seddonym Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth using benchmark.pedantic for each of these, like with the others, so we get to average out the runs. That will only apply to a local run.

Copy link
Collaborator Author

@Peter554 Peter554 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done it.

🐼 I'm not fully sure what this does. Even without pedantic pytest-benchmark will run many iterations and take an average. What's the difference between between iterations and rounds here?

result = _run_benchmark(benchmark, large_graph.find_descendants, "mypackage")
assert len(result) == 28222
Copy link
Collaborator Author

@Peter554 Peter554 Jan 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @seddonym - see here that fixing the graph increases the number of modules. I.e. we were missing many modules before.



def test_find_downstream_modules(large_graph, benchmark):
result = _run_benchmark(
benchmark, large_graph.find_downstream_modules, DEEP_LAYERS[0], as_package=True
)
assert len(result) == 80


def test_find_upstream_modules(large_graph, benchmark):
result = _run_benchmark(
benchmark, large_graph.find_upstream_modules, DEEP_LAYERS[0], as_package=True
)
assert len(result) == 2159


class TestFindShortestChain:
def test_chain_found(self, large_graph, benchmark):
result = _run_benchmark(
benchmark, large_graph.find_shortest_chain, DEEP_LAYERS[0], DEEP_LAYERS[1]
)
assert result is not None

def test_no_chain(self, large_graph, benchmark):
result = _run_benchmark(
benchmark,
large_graph.find_shortest_chain,
DEEP_LAYERS[0],
"mypackage.data.vendors.4053192739.6373932949",
)
assert result is None


class TestFindShortestChains:
def test_chains_found(self, large_graph, benchmark):
result = _run_benchmark(
benchmark,
large_graph.find_shortest_chains,
DEEP_LAYERS[0],
DEEP_LAYERS[1],
as_packages=True,
)
assert len(result) > 0

def test_no_chains(self, large_graph, benchmark):
result = _run_benchmark(
benchmark,
large_graph.find_shortest_chains,
DEEP_LAYERS[0],
"mypackage.data.vendors.4053192739.6373932949",
as_packages=True,
)
assert result == set()
Loading