Skip to content
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

ReBenchLog gauge adapter doesn't work on case-sensitive filesystems #199

Closed
raehik opened this issue Jan 23, 2023 · 7 comments · Fixed by #202
Closed

ReBenchLog gauge adapter doesn't work on case-sensitive filesystems #199

raehik opened this issue Jan 23, 2023 · 7 comments · Fixed by #202

Comments

@raehik
Copy link
Contributor

raehik commented Jan 23, 2023

ReBench's custom benchmark log format is defined in interop/rebench_log_adapter.py, in the class RebenchLogAdapter. The corresponding benchmark_suites.${benchmark}.gauge_adapter value is defined in executor.py:

ReBench/rebench/executor.py

Lines 446 to 449 in 08c736b

def _get_gauge_adapter_instance(self, adapter_name):
adapter_name += "Adapter"
root = sys.modules['rebench.interop'].__path__

So the correct value is RebenchLog. However, the documentation and code reference ReBenchLog a number of times. On my Linux-based system, that fails to find the gauge adapter. I hazard that it would work on Windows (but probably not MacOS?).

In particular, docs/extensions.md states the wrong value.

## Available Harness Support
ReBench currently provides builtin support for the following benchmark harnesses:
- `JMH`: [JMH](http://openjdk.java.net/projects/code-tools/jmh/), Java's microbenchmark harness
- `PlainSecondsLog`: a plain seconds log, i.e., a floating point number per line
- `ReBenchLog`: the ReBench log format, which indicates benchmark name and run time in milliseconds or microseconds
- `SavinaLog`: the harness of the [Savina](https://github.com/shamsimam/savina) benchmarks
- `ValidationLog`: the format used by [SOMns](https://github.com/smarr/SOMns)'s ImpactHarness
- `Time`: a harness that uses `/usr/bin/time` automatically

@raehik
Copy link
Contributor Author

raehik commented Jan 23, 2023

Wonder if this can easily be made case-insensitive?

@smarr
Copy link
Owner

smarr commented Jan 23, 2023

My reading of https://python.readthedocs.io/en/latest/library/importlib.html and https://peps.python.org/pep-0235/ is that Python is a case-sensitive language, and I didn't see any option to use case insensitive semantics for the import.

So, I fear that's a documentation bug. 🙁

@raehik
Copy link
Contributor Author

raehik commented Jan 23, 2023

Hmm. I imagine it could be done with some string checking machinery, a masterlist of strings to gauge formats (instead of importing dynamically). Is that worthwhile, or should I amend the docs and add a help message for when Python gets an invalid gauge adapter?

@smarr
Copy link
Owner

smarr commented Jan 23, 2023

Hm, right. We could intercept well known names, check them case insensitive, and map them to the corresponding and correctly cased name.

Perhaps there's even no need for the dynamic import.
The current model is designed to make it easy to just drop in a suitable file.
But, in practice, none has a custom one.
It's also not really practical since if one would install ReBench via pip, one probably would want to have the gauge adapter somehow separate, possibly next to the benchmark config.

Give that rather suboptimal situation, we could try something like looking for gauge adapters on the disk, and then check case insensitively whether the names match, and use the file name we found on the disk for import.

@smarr
Copy link
Owner

smarr commented Jan 23, 2023

And, yeah, one could possibly indeed drop the dynamic import altogether until someone actually asks for it.

@raehik
Copy link
Contributor Author

raehik commented Jan 24, 2023

It wasn't immediately apparent how to make gauge adapter resolution case insensitive (it's done by attempting to import packages until one hits, not comparing strings/filenames), so I amended some docs and added a more useful error message on failure in #200 .

@smarr
Copy link
Owner

smarr commented Feb 2, 2023

Now implemented as case-insensitive with #202.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants