Skip to content

fix: use json.loads with Path.read_text() instead of json.load with Path#10361

Open
satishkc7 wants to merge 2 commits intohiyouga:mainfrom
satishkc7:fix/json-load-path-argument
Open

fix: use json.loads with Path.read_text() instead of json.load with Path#10361
satishkc7 wants to merge 2 commits intohiyouga:mainfrom
satishkc7:fix/json-load-path-argument

Conversation

@satishkc7
Copy link
Copy Markdown

Summary

json.load() expects a file-like object, not a pathlib.Path. Passing a Path directly raises:

AttributeError: 'PosixPath' object has no attribute 'read'

This happens whenever a .json config file is passed as the first CLI argument (e.g. llamafactory-cli train config.json).

Root Cause

In src/llamafactory/hparams/parser.py, the JSON branch of read_args():

# Before (broken)
dict_config = OmegaConf.create(json.load(Path(sys.argv[1]).absolute()))

The YAML branch correctly uses OmegaConf.load() which accepts Path objects, but json.load() does not.

Fix

# After (fixed)
dict_config = OmegaConf.create(json.loads(Path(sys.argv[1]).absolute().read_text()))

Using Path.read_text() reads the file content as a string, and json.loads() parses a string - no file object needed.

Fixes #10316

json.load() expects a file-like object, not a pathlib.Path. Passing a
Path directly raises AttributeError: 'PosixPath' object has no
attribute 'read'. Replace with json.loads(path.read_text()) to
correctly read the JSON config file.

Fixes hiyouga#10316
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the JSON configuration loading logic in parser.py to read file contents as text before parsing. A review comment suggests using OmegaConf.load() instead to improve consistency with YAML handling and simplify the implementation.

elif len(sys.argv) > 1 and sys.argv[1].endswith(".json"):
override_config = OmegaConf.from_cli(sys.argv[2:])
dict_config = OmegaConf.create(json.load(Path(sys.argv[1]).absolute()))
dict_config = OmegaConf.create(json.loads(Path(sys.argv[1]).absolute().read_text()))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since OmegaConf.load() is already used for YAML files and it natively supports JSON, you can use it here as well. This improves consistency with the YAML loading logic and simplifies the code by avoiding the intermediate string conversion and explicit json.loads() call. Note that if you apply this change, the import json on line 18 will become unused and can be removed.

dict_config = OmegaConf.load(Path(sys.argv[1]).absolute())

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 this pull request may close these issues.

'PosixPath' object has no attribute 'read'

1 participant