Fix --log-file handling and correct stale CLI docs - #26
Open
youngdashu wants to merge 3 commits into
Open
Conversation
The top-level parser and the sub-parsers both defined -v/--verbose and --log-file. Because the sub-parser defaults overwrite the values that the top-level parser puts in the namespace, `fridata --log-file x.log generate_embeddings ...` silently dropped the log file. Move both flags into a shared add_logging_arguments() that sets default=argparse.SUPPRESS, so a flag that is not given after the sub-command keeps the top-level value. This also adds --log-file to create_dataset and generate_data, which had no such flag, and removes a duplicate -v/--verbose definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The src/ layout removed fridata.py, so the AFDB example must call `python -m fridata` with PYTHONPATH pointing at src/. demo.ipynb used two forms that the CLI does not accept: `create_dashboard --dataset X` (the argument is positional) and `fridata load`, a sub-command that no longer exists. Replace the second one with a short Python snippet that prints the same dataset object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setup_env.sh used whatever python3 was on PATH. If that interpreter was older than the requires-python floor (a conda base environment, for example), the script created the venv and only failed later, inside pip. Auto-detect now tries python3.14 down to python3.11 before plain python3/python and takes the first one that is at least 3.11. A new --python EXEC option (or PYTHON=...) selects an interpreter explicitly and is rejected up front when it is too old. The floor lives in MIN_MAJOR/MIN_MINOR, to be kept in sync with pyproject.toml. Also ignore .venv/ and venv/, and document the new option in the README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Three changes.
--log-filebefore the sub-command — the top-level parser and the sub-parsers both defined-v/--verboseand--log-file, and the sub-parser defaults overwrote the values that the top-level parser had put in the namespace. Sofridata --log-file x.log generate_embeddings ...silently dropped the log file. Both flags now come from a sharedadd_logging_arguments()withdefault=argparse.SUPPRESS, so a flag given before the sub-command survives. This also givescreate_datasetandgenerate_dataa--log-fileflag, which they never had, and removes a duplicate-v/--verbosedefinition.Stale CLI invocations in the docs — the src/ layout removed
fridata.py, so the AFDB example in the README and on the docs page now callspython -m fridata.demo.ipynbused two forms the CLI does not accept:create_dashboard --dataset X(the argument is positional) andfridata load, a sub-command that no longer exists; the latter is replaced by a short Python snippet that prints the same dataset object.scripts/setup_env.shinterpreter choice — the script took whateverpython3was onPATHand then failed inside pip when that interpreter was older than therequires-pythonfloor (a conda base environment, for example). It now prefers the newest Python >= 3.11 that it finds, accepts--python EXEC(orPYTHON=...) and rejects a too-old interpreter up front..venv/andvenv/are now ignored.Testing
Local run on Python 3.13 with
pip install -e ".[embeddings,test]": 137 passed, 1 skipped. Every command in the corrected docs was checked againstcreate_parser().