Feature/jschen 20260702 aalcr and ifbench#395
Conversation
jschen069
temporarily deployed
to
smoke-test-approval
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the AA-LCR and IFBench datasets, including configurations, dataset loaders, evaluators, and unit tests. The review feedback identifies several critical issues to address: resolving hardcoded paths dynamically in aa_lcr.py to support package-installed runs, guarding against potential IndexError and KeyError exceptions in ifbench.py when handling mismatched or missing dataset fields, and preventing test suite crashes by guarding site.getsitepackages() in environments where it is unavailable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| dict( | ||
| abbr='aa_lcr', | ||
| type=AALCRDataset, | ||
| path='benchmark/ais_bench/datasets/aa_lcr/', |
There was a problem hiding this comment.
The dataset path is configured as 'benchmark/ais_bench/datasets/aa_lcr/'. However, relative paths in the benchmark configuration should be relative to the repository root (starting with ais_bench/), similar to other datasets (e.g., ifbench). Including the repository folder name benchmark as a prefix will cause file-not-found errors when running the benchmark from the repository root.
| path='benchmark/ais_bench/datasets/aa_lcr/', | |
| path='ais_bench/datasets/aa_lcr/', |
| def _ensure_text_dir_downloaded() -> Path: | ||
| """Ensure AA-LCR extracted texts are available locally. | ||
|
|
||
| Looks for the document corpus ZIP at the relative path | ||
| ``benchmark/ais_bench/datasets/aa_lcr/extracted_text/AA-LCR_extracted-text.zip`` | ||
| (sibling to this file in the repository), extracts it into the cache | ||
| directory on first use, and returns the path to the ``lcr/`` directory | ||
| containing the ``.txt`` files. Subsequent calls return the cached path | ||
| immediately. | ||
|
|
||
| This mirrors evalscope's ``_ensure_text_dir_downloaded()`` but reads | ||
| from a local ZIP instead of downloading from ModelScope. | ||
|
|
||
| Returns: | ||
| Path to the ``lcr/`` directory containing extracted ``.txt`` files. | ||
|
|
||
| Raises: | ||
| FileNotFoundError: If the local ZIP file does not exist. | ||
| ValueError: If extraction fails. | ||
| """ | ||
| cache_root = Path(get_cache_dir(DEFAULT_CACHE_ROOT)) / DEFAULT_CACHE_SUBDIR | ||
| extracted_dir = cache_root / DEFAULT_EXTRACTED_DIR_NAME | ||
|
|
||
| if extracted_dir.exists(): | ||
| logger.info(f'AA-LCR documents found in cache: {extracted_dir}') | ||
| return extracted_dir | ||
|
|
||
| local_zip = Path(_DOC_ZIP_PATH) | ||
| if not local_zip.exists(): | ||
| raise FileNotFoundError( | ||
| f'AA-LCR document corpus ZIP not found at: {local_zip}\n' | ||
| 'Please ensure the file is placed at ' | ||
| 'benchmark/ais_bench/datasets/aa_lcr/extracted_text/' | ||
| 'AA-LCR_extracted-text.zip relative to the repository root.' | ||
| ) | ||
|
|
||
| cache_root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
_ensure_text_dir_downloaded currently uses module-level constants _DOC_ZIP_PATH and DEFAULT_CACHE_ROOT which are hardcoded relative to the package directory. This will fail when the package is installed in site-packages (which is often read-only) and the dataset is in the user's workspace. We should pass the configured path to _ensure_text_dir_downloaded and resolve the paths dynamically.
def _ensure_text_dir_downloaded(path: str = None) -> Path:
"""Ensure AA-LCR extracted texts are available locally.
Looks for the document corpus ZIP at the relative path
``benchmark/ais_bench/datasets/aa_lcr/extracted_text/AA-LCR_extracted-text.zip``
(sibling to this file in the repository), extracts it into the cache
directory on first use, and returns the path to the ``lcr/`` directory
containing the ``.txt`` files. Subsequent calls return the cached path
immediately.
This mirrors evalscope's ``_ensure_text_dir_downloaded()`` but reads
from a local ZIP instead of downloading from ModelScope.
Returns:
Path to the ``lcr/`` directory containing extracted ``.txt`` files.
Raises:
FileNotFoundError: If the local ZIP file does not exist.
ValueError: If extraction fails.
"""
data_dir = get_data_path(path, local_mode=True) if path else _DATA_DIR
cache_root = Path(get_cache_dir(os.path.dirname(data_dir))) / DEFAULT_CACHE_SUBDIR
extracted_dir = cache_root / DEFAULT_EXTRACTED_DIR_NAME
if extracted_dir.exists():
logger.info(f'AA-LCR documents found in cache: {extracted_dir}')
return extracted_dir
local_zip = Path(data_dir) / 'extracted_text' / 'AA-LCR_extracted-text.zip'
if not local_zip.exists():
raise FileNotFoundError(
f'AA-LCR document corpus ZIP not found at: {local_zip}\n'
'Please ensure the file is placed at '
'benchmark/ais_bench/datasets/aa_lcr/extracted_text/'
'AA-LCR_extracted-text.zip relative to the repository root.'
)
cache_root.mkdir(parents=True, exist_ok=True)| csv_path = _CSV_PATH | ||
| if path and os.path.isabs(path): | ||
| candidate = os.path.join(path, 'AA-LCR_Dataset.csv') | ||
| if os.path.exists(candidate): | ||
| csv_path = candidate |
There was a problem hiding this comment.
When ais_bench is installed as a package, _CSV_PATH (which is defined relative to _THIS_DIR) will point to the package installation directory (e.g., in site-packages), whereas the user's downloaded datasets reside in the workspace directory. To support package-installed runs, we should use get_data_path to dynamically resolve the dataset path.
| csv_path = _CSV_PATH | |
| if path and os.path.isabs(path): | |
| candidate = os.path.join(path, 'AA-LCR_Dataset.csv') | |
| if os.path.exists(candidate): | |
| csv_path = candidate | |
| if path: | |
| path = get_data_path(path, local_mode=True) | |
| csv_path = os.path.join(path, 'AA-LCR_Dataset.csv') | |
| else: | |
| csv_path = _CSV_PATH |
| kwargs = inp.kwargs[index] | ||
| logger.info(f"[ifbench][strict] kwargs[{index}]: {kwargs}") | ||
|
|
||
| instruction.build_description(**kwargs) |
There was a problem hiding this comment.
If inp.kwargs has fewer elements than instruction_list (e.g., due to a malformed dataset item), accessing inp.kwargs[index] will raise an IndexError and crash the evaluation. We should guard against this defensively.
| kwargs = inp.kwargs[index] | |
| logger.info(f"[ifbench][strict] kwargs[{index}]: {kwargs}") | |
| instruction.build_description(**kwargs) | |
| kwargs = inp.kwargs[index] if index < len(inp.kwargs) else {} | |
| logger.info(f"[ifbench][strict] kwargs[{index}]: {kwargs}") | |
| instruction.build_description(**kwargs) |
| instruction.build_description(**inp.kwargs[index]) | ||
| args = instruction.get_instruction_args() | ||
| logger.info(f"[ifbench][loose] kwargs[{index}]: {inp.kwargs[index]}") | ||
| logger.info(f"[ifbench][loose] args: {args}") |
There was a problem hiding this comment.
Similarly, guard against IndexError when accessing inp.kwargs[index] in loose mode.
| instruction.build_description(**inp.kwargs[index]) | |
| args = instruction.get_instruction_args() | |
| logger.info(f"[ifbench][loose] kwargs[{index}]: {inp.kwargs[index]}") | |
| logger.info(f"[ifbench][loose] args: {args}") | |
| kwargs = inp.kwargs[index] if index < len(inp.kwargs) else {} | |
| instruction.build_description(**kwargs) | |
| args = instruction.get_instruction_args() | |
| logger.info(f"[ifbench][loose] kwargs[{index}]: {kwargs}") | |
| logger.info(f"[ifbench][loose] args: {args}") |
| def __init__(self, reader_cfg=None, k=1, n=1, task_state_manager=None, **kwargs): | ||
| # Ensure the document corpus is available *before* BaseDataset | ||
| # calls self.load(), which needs the text_dir. | ||
| self.text_dir = _ensure_text_dir_downloaded() | ||
| super().__init__(reader_cfg=reader_cfg or {}, k=k, n=n, task_state_manager=task_state_manager, **kwargs) |
There was a problem hiding this comment.
Pass the configured path from kwargs to _ensure_text_dir_downloaded to ensure the zip file and cache directory are resolved correctly relative to the workspace.
| def __init__(self, reader_cfg=None, k=1, n=1, task_state_manager=None, **kwargs): | |
| # Ensure the document corpus is available *before* BaseDataset | |
| # calls self.load(), which needs the text_dir. | |
| self.text_dir = _ensure_text_dir_downloaded() | |
| super().__init__(reader_cfg=reader_cfg or {}, k=k, n=n, task_state_manager=task_state_manager, **kwargs) | |
| def __init__(self, reader_cfg=None, k=1, n=1, task_state_manager=None, **kwargs): | |
| # Ensure the document corpus is available *before* BaseDataset | |
| # calls self.load(), which needs the text_dir. | |
| path = kwargs.get('path') | |
| self.text_dir = _ensure_text_dir_downloaded(path) | |
| super().__init__(reader_cfg=reader_cfg or {}, k=k, n=n, task_state_manager=task_state_manager, **kwargs) |
| 'relative to the repository root.' | ||
| ) | ||
|
|
||
| text_dir = _ensure_text_dir_downloaded() |
There was a problem hiding this comment.
Pass path to _ensure_text_dir_downloaded in load as well.
| text_dir = _ensure_text_dir_downloaded() | |
| text_dir = _ensure_text_dir_downloaded(path) |
| inp = InputExample( | ||
| key=refer['key'], | ||
| instruction_id_list=refer['instruction_id_list'], | ||
| prompt=refer['prompt'], | ||
| kwargs=[ | ||
| {k: v for k, v in kwarg.items() if v is not None} | ||
| for kwarg in refer.get('kwargs', []) | ||
| ]) |
There was a problem hiding this comment.
Accessing keys like 'key', 'instruction_id_list', and 'prompt' directly using refer[...] can raise a KeyError if the dataset schema changes or contains unexpected missing fields. Using .get() with safe defaults is more robust.
| inp = InputExample( | |
| key=refer['key'], | |
| instruction_id_list=refer['instruction_id_list'], | |
| prompt=refer['prompt'], | |
| kwargs=[ | |
| {k: v for k, v in kwarg.items() if v is not None} | |
| for kwarg in refer.get('kwargs', []) | |
| ]) | |
| inp = InputExample( | |
| key=refer.get('key', 0), | |
| instruction_id_list=refer.get('instruction_id_list', []), | |
| prompt=refer.get('prompt', ''), | |
| kwargs=[ | |
| {k: v for k, v in kwarg.items() if v is not None} | |
| for kwarg in refer.get('kwargs', []) | |
| ]) |
| for _sp in _site.getsitepackages(): | ||
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | ||
| if os.path.isfile(_candidate): | ||
| _hf_spec = _iu.spec_from_file_location( | ||
| "datasets", _candidate, | ||
| submodule_search_locations=[os.path.dirname(_candidate)] | ||
| ) | ||
| _hf = _iu.module_from_spec(_hf_spec) | ||
| sys.modules["datasets"] = _hf | ||
| _hf_spec.loader.exec_module(_hf) | ||
| break |
There was a problem hiding this comment.
site.getsitepackages() is not always available in all Python environments (e.g., some virtual environments or custom builds). Calling it directly without checking can cause the entire test suite to crash with an AttributeError. We should guard against this.
| for _sp in _site.getsitepackages(): | |
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | |
| if os.path.isfile(_candidate): | |
| _hf_spec = _iu.spec_from_file_location( | |
| "datasets", _candidate, | |
| submodule_search_locations=[os.path.dirname(_candidate)] | |
| ) | |
| _hf = _iu.module_from_spec(_hf_spec) | |
| sys.modules["datasets"] = _hf | |
| _hf_spec.loader.exec_module(_hf) | |
| break | |
| getsitepackages = getattr(_site, "getsitepackages", None) | |
| if getsitepackages: | |
| for _sp in getsitepackages(): | |
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | |
| if os.path.isfile(_candidate): | |
| _hf_spec = _iu.spec_from_file_location( | |
| "datasets", _candidate, | |
| submodule_search_locations=[os.path.dirname(_candidate)] | |
| ) | |
| _hf = _iu.module_from_spec(_hf_spec) | |
| sys.modules["datasets"] = _hf | |
| _hf_spec.loader.exec_module(_hf) | |
| break |
| for _sp in _site.getsitepackages(): | ||
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | ||
| if os.path.isfile(_candidate): | ||
| _hf_spec = _iu.spec_from_file_location( | ||
| "datasets", _candidate, | ||
| submodule_search_locations=[os.path.dirname(_candidate)] | ||
| ) | ||
| _hf = _iu.module_from_spec(_hf_spec) | ||
| sys.modules["datasets"] = _hf | ||
| _hf_spec.loader.exec_module(_hf) | ||
| break |
There was a problem hiding this comment.
Similarly, guard site.getsitepackages() in test_ifbench.py to prevent test suite crashes in virtual environments.
| for _sp in _site.getsitepackages(): | |
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | |
| if os.path.isfile(_candidate): | |
| _hf_spec = _iu.spec_from_file_location( | |
| "datasets", _candidate, | |
| submodule_search_locations=[os.path.dirname(_candidate)] | |
| ) | |
| _hf = _iu.module_from_spec(_hf_spec) | |
| sys.modules["datasets"] = _hf | |
| _hf_spec.loader.exec_module(_hf) | |
| break | |
| getsitepackages = getattr(_site, "getsitepackages", None) | |
| if getsitepackages: | |
| for _sp in getsitepackages(): | |
| _candidate = os.path.join(_sp, "datasets", "__init__.py") | |
| if os.path.isfile(_candidate): | |
| _hf_spec = _iu.spec_from_file_location( | |
| "datasets", _candidate, | |
| submodule_search_locations=[os.path.dirname(_candidate)] | |
| ) | |
| _hf = _iu.module_from_spec(_hf_spec) | |
| sys.modules["datasets"] = _hf | |
| _hf_spec.loader.exec_module(_hf) | |
| break |
jschen069
had a problem deploying
to
smoke-test-approval
GitHub Actions
Failure
…m:jschen069/benchmark into feature/jschen_20260702_aalcr_and_ifbench
jschen069
had a problem deploying
to
smoke-test-approval
GitHub Actions
Error
jschen069
had a problem deploying
to
smoke-test-approval
GitHub Actions
Error
2 participants
Thanks for your contribution; we appreciate it a lot. The following instructions will make your pull request healthier and help you get feedback more easily. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
感谢您的贡献，我们非常重视。以下说明将使您的拉取请求更健康，更易于获得反馈。如果您不理解某些项目，请不要担心，只需提交拉取请求并从维护人员那里寻求帮助即可。
PR Type / PR类型
Related Issue | 关联 Issue
Fixes #(issue ID / issue 编号) / Relates to #(issue ID / issue 编号)
🔍 Motivation / 变更动机
Please describe the motivation of this PR and the goal you want to achieve through this PR.
请描述您的拉取请求的动机和您希望通过此拉取请求实现的目标。
📝 Modification / 修改内容
Please briefly describe what modification is made in this PR.
请简要描述此拉取请求中进行的修改。
📐 Associated Test Results / 关联测试结果
Please provide links to the related test results, such as CI pipelines, test reports, etc.
请提供相关测试结果的链接，例如 CI 管道、测试报告等。
Does the modification introduce changes that break the backward compatibility of the downstream repositories? If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
是否引入了会破坏下游存储库向后兼容性的更改？如果是，请描述它如何破坏兼容性，以及下游项目应该如何修改其代码以保持与此 PR 的兼容性。
If the modification introduces performance degradation, please describe the impact of the performance degradation and the expected performance improvement.
如果引入了性能下降，请描述性能下降的影响和预期的性能改进。
🌟 Use cases (Optional) / 使用案例（可选）
If this PR introduces a new feature, it is better to list some use cases here and update the documentation.
如果此拉取请求引入了新功能，最好在此处列出一些用例并更新文档。
✅ Checklist / 检查列表
Before PR:
After PR:
👥 Collaboration Info / 协作信息
🌟 Useful CI Command / 实用的CI命令
/gemini review/gemini summary/gemini help/readthedocs build