fix(helm): remove ':' from test fixture paths so repo is cloneable on Windows#201
Open
Mandark-droid wants to merge 1 commit into
Open
fix(helm): remove ':' from test fixture paths so repo is cloneable on Windows#201Mandark-droid wants to merge 1 commit into
Mandark-droid wants to merge 1 commit into
Conversation
… Windows The HELM test fixture directories used HELM run-spec names verbatim as directory names, which contain ':' — an invalid character in Windows filenames. This made `git clone` fail at checkout on Windows (error: invalid path 'tests/data/helm/commonsense:dataset=...'). Rename the three fixture directories, replacing ':' with '_', and update the path references in the HELM tests, the converters README example, and the helm converter's default --log_path. File contents are unchanged (pure renames); nothing at runtime parses these directory names — the adapter reads run_spec.json from inside each run directory. Fixes evaleval#200
Mandark-droid
force-pushed
the
fix/windows-colon-fixture-paths
branch
from
bcbfdc6 to
7679e88
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #200
Problem
The HELM test fixtures use HELM run-spec names verbatim as directory names, and those contain
:— an illegal character in Windows filenames. As a result,git clonefails at checkout on Windows:Change
tests/data/helm/, replacing:with_(pure renames, 100% similarity — file contents untouched).every_eval_ever/converters/README.md, and the default--log_pathinevery_eval_ever/converters/helm/__main__.py.Nothing at runtime parses these directory names — the adapter reads
run_spec.jsonfrom inside each run directory — so real HELM output directories (which do contain:on Linux/macOS) are unaffected.Verification
On Windows 11 + Python 3.12 (where this repo previously could not even be checked out):
pytest tests/test_helm_adapter.py tests/test_helm_instance_level_adapter.py→ 19 passed, including all 16 fixture-dependent tests.every_eval_ever convert helm --log_path tests/data/helm/commonsense_dataset=...) converts successfully.(Aside noticed while testing, unrelated to this change: on Windows the test run needs
PYTHONUTF8=1because crfm-helm reads files with the locale encoding, and on Python 3.14 crfm-helm fails to import entirely due to its pydantic-v1 usage — the tests then skip gracefully.)A follow-up could add a CI check rejecting committed paths containing characters invalid on Windows (
: * ? " < > |) to prevent regressions — happy to do that in a separate PR if wanted.