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

CLI cleanup and documentation #2244

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

CLI cleanup and documentation #2244

wants to merge 11 commits into from

Conversation

djsaunde
Copy link
Contributor

@djsaunde djsaunde commented Jan 8, 2025

Description

This PR implements a cleanup / modularization of the code under axolotl/cli, where previously, many of the functions were lumped together in the axolotl/cli/__init__.py file.

Motivation and Context

These changes allow for better organization / readability and decluttering of the axolotl.cli namespace.

How has this been tested?

CLI tests pass.

Types of changes

Added doc strings, removed dead code, refactored methods etc. out of axolotl/cli/__init__.py into aptly-named modules.

src/axolotl/cli/art.py Outdated Show resolved Hide resolved
def preprocess(config: str, **kwargs):
"""Preprocess datasets before training."""
kwargs = {k: v for k, v in kwargs.items() if v is not None}
@filter_none_kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

noob q: why do we need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The filter_none_kwargs wrapper? It removes arguments that have None values, which were added by my Click option adding wrappers. I think we could probably just modify the Click option adding wrappers but this works for now.

src/axolotl/common/cli.py Outdated Show resolved Hide resolved
src/axolotl/common/datasets.py Outdated Show resolved Hide resolved
@SalmanMohammadi
Copy link
Contributor

I had a quick first skim, this looks great. Thanks for cleaning this up, the cli module is substantially more readable.
I'll come back tomorrow with some more comments - mostly a whole bucket of nits.

src/axolotl/cli/checks.py Outdated Show resolved Hide resolved
src/axolotl/cli/config.py Outdated Show resolved Hide resolved
src/axolotl/cli/config.py Outdated Show resolved Hide resolved
src/axolotl/cli/preprocess.py Outdated Show resolved Hide resolved
src/axolotl/common/datasets.py Outdated Show resolved Hide resolved
cli_args: Command line arguments
dataset_meta: Dataset metadata containing training and evaluation datasets
cfg: Config dictionary.
dataset_meta: Dataset metadata containing training and evaluation datasets.

Returns:
Tuple containing:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, why?

@SalmanMohammadi
Copy link
Contributor

Various minor nits, but LGTM! I'll let the others review+stamp.

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.

3 participants