-
Notifications
You must be signed in to change notification settings - Fork 152
Pre-kompile K definitions and specs for integration tests #2417
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.
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
Changes from all commits
0746d93
0a01455
ee39882
79bc8c5
1fbf00d
0d01474
b91a09b
9ccd0a8
37b10fd
870f723
cb0e941
00bb4b9
0056a78
792b7c1
c49ab2a
9086efd
5d8f909
aada391
4a124e0
b30fd19
3701ddc
ef47d11
5ce032d
3a9d51f
0ab98ac
75852dc
6981c85
54c5c4a
87b7891
ed881e7
19c4b81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api" | |
|
||
[tool.poetry] | ||
name = "kevm-pyk" | ||
version = "1.0.547" | ||
version = "1.0.548" | ||
description = "" | ||
authors = [ | ||
"Runtime Verification, Inc. <[email protected]>", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,5 +5,4 @@ | |
if TYPE_CHECKING: | ||
from typing import Final | ||
|
||
|
||
VERSION: Final = '1.0.547' | ||
VERSION: Final = '1.0.548' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
from typing import TYPE_CHECKING, NamedTuple | ||
|
||
import pytest | ||
from filelock import FileLock | ||
from pyk.prelude.ml import is_top | ||
from pyk.proof.reachability import APRProof | ||
|
||
|
@@ -21,7 +22,7 @@ | |
from typing import Any, Final | ||
|
||
from pyk.utils import BugReport | ||
from pytest import LogCaptureFixture, TempPathFactory | ||
from pytest import FixtureRequest, LogCaptureFixture | ||
|
||
|
||
sys.setrecursionlimit(10**8) | ||
|
@@ -118,31 +119,58 @@ class Target(NamedTuple): | |
main_file: Path | ||
main_module_name: str | ||
|
||
@property | ||
def name(self) -> str: | ||
""" | ||
Target's name is the two trailing path segments and the main module name | ||
""" | ||
return f'{self.main_file.parts[-2]}-{self.main_file.stem}-{self.main_module_name}' | ||
|
||
def __call__(self, output_dir: Path) -> Path: | ||
return kevm_kompile( | ||
output_dir=output_dir / 'kompiled', | ||
target=KompileTarget.HASKELL, | ||
main_file=self.main_file, | ||
main_module=self.main_module_name, | ||
syntax_module=self.main_module_name, | ||
debug=True, | ||
) | ||
with FileLock(str(output_dir) + '.lock'): | ||
return kevm_kompile( | ||
output_dir=output_dir / 'kompiled', | ||
target=KompileTarget.HASKELL, | ||
main_file=self.main_file, | ||
main_module=self.main_module_name, | ||
syntax_module=self.main_module_name, | ||
debug=True, | ||
) | ||
|
||
|
||
@pytest.fixture(scope='module') | ||
def kompiled_target_cache(kompiled_targets_dir: Path) -> tuple[Path, dict[str, Path]]: | ||
""" | ||
Populate the cache of kompiled definitions from an existing file system directory. If the cache is hot, the `kompiled_target_for` fixture will not containt a call to `kompile`, saving an expesive call to the K frontend. | ||
""" | ||
cache_dir = kompiled_targets_dir / 'target' | ||
cache: dict[str, Path] = {} | ||
if cache_dir.exists(): # cache dir exists, populate cache | ||
for file in cache_dir.iterdir(): | ||
if file.is_dir(): | ||
# the cache key is the name of the target, which is the filename by-construction. | ||
cache[file.stem] = file / 'kompiled' | ||
else: | ||
cache_dir.mkdir(parents=True) | ||
return cache_dir, cache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider abstracting this in a class. |
||
|
||
|
||
@pytest.fixture(scope='module') | ||
def kompiled_target_for(tmp_path_factory: TempPathFactory) -> Callable[[Path], Path]: | ||
cache_dir = tmp_path_factory.mktemp('target') | ||
cache: dict[Target, Path] = {} | ||
def kompiled_target_for(kompiled_target_cache: tuple[Path, dict[str, Path]]) -> Callable[[Path], Path]: | ||
""" | ||
Generate a function that returns a path to the kompiled defintion for a given K spec. Invoke `kompile` only if no kompiled directory is cached for the spec. | ||
""" | ||
cache_dir, cache = kompiled_target_cache | ||
|
||
def kompile(spec_file: Path) -> Path: | ||
target = _target_for_spec(spec_file) | ||
|
||
if target not in cache: | ||
output_dir = cache_dir / f'{target.main_file.stem}-{len(cache)}' | ||
output_dir.mkdir() | ||
cache[target] = target(output_dir) | ||
if target.name not in cache: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not possible to construct a |
||
output_dir = cache_dir / target.name | ||
output_dir.mkdir(exist_ok=True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to cover the case when the user points to an already existing cache to rekompile it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, turns out it is actually easier if we keep |
||
cache[target.name] = target(output_dir) | ||
|
||
return cache[target] | ||
return cache[target.name] | ||
|
||
return kompile | ||
|
||
|
@@ -156,6 +184,32 @@ def _target_for_spec(spec_file: Path) -> Target: | |
return Target(main_file, main_module_name) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
'spec_file', | ||
ALL_TESTS, | ||
ids=[str(spec_file.relative_to(SPEC_DIR)) for spec_file in ALL_TESTS], | ||
) | ||
def test_kompile_targets(spec_file: Path, kompiled_target_for: Callable[[Path], Path], request: FixtureRequest) -> None: | ||
""" | ||
This test function is intended to be used to pre-kompile all definitions, | ||
so that the actual proof tests do not need to do the actual compilation, | ||
which is disturbing performance measurment. | ||
|
||
To achieve the desired caching, this test should be run like this: | ||
pytest src/tests/integration/test_prove.py::test_kompile_targets --kompiled-targets-dir ./prekompiled | ||
|
||
This test will be skipped if no --kompiled-targets-dir option is given | ||
""" | ||
dir = request.config.getoption('--kompiled-targets-dir') | ||
if not dir: | ||
pytest.skip() | ||
|
||
if spec_file in FAILING_BOOSTER_TESTS: | ||
pytest.skip() | ||
|
||
kompiled_target_for(spec_file) | ||
|
||
|
||
# --------- | ||
# Pyk tests | ||
# --------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
1.0.547 | ||
1.0.548 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing a cache might be unnecessary, you can just do the directory lookup when you request the kompiled directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean: could you elaborate?
My goals are:
cache
in so that the old behavior of thekompiled_target_for
fixture is retained,cache
is never actually updated when--kompiled-targets-dir
is passed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it, tests basically require an interface that takes a
Target
and returns aPath
to the kompiled target (in the actual tests there might be another indirection from spec files to targets, but I'll not consider this for simplicity).So you just have to ensure that
Target -> Path
is reproducible between runs, then accommodating both use cases (prekompiled vs on-the-fly) is straightforward.Path
does not exist, lock and kompilePath
.For prekompiled mode, you pass in a non-empty directory as base. For on-the-file mode, you pass in an empty temporary directory.
cache
, etc. can be dropped, the actual cache is the file system.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sound good, I understand what you mean. However, I'd rather get my incremental change merged quickly and now spend too much time refactoring the test suite. Would that be OK if I let you refactor it as you see fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with that, but please open an issue for the refactoring and assign it to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will do!