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

Documentation and typing of functions #5

Merged
merged 15 commits into from
Feb 6, 2024
Merged

Conversation

NathanHB
Copy link
Member

No description provided.

@NathanHB NathanHB requested a review from clefourrier January 31, 2024 13:03
Copy link
Member

@clefourrier clefourrier left a comment

Choose a reason for hiding this comment

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

2 important style things:

  • Remove all the The at the start of arg docstrings. It makes them heavier to read.
    Ex: instead of The revision of the model, use Model revision (commit hash).
  • Follow the ref doc for optional items (try to be homogeneous)

One comment:

  • Try to add and explain the small edge cases in the doc - why are we asking for the non obvious params? what happens when a param is None? Etc.

src/lighteval/logging/info_loggers.py Outdated Show resolved Hide resolved
src/lighteval/models/model_config.py Show resolved Hide resolved
src/lighteval/models/model_config.py Outdated Show resolved Hide resolved
Base configuration class for models.

Attributes:
pretrained (str): The HuggingFace Hub model ID name or the path to a
Copy link
Member

Choose a reason for hiding this comment

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

  • Remove all the The at the start of arg docstrings.
    Ex: instead of The revision of the model, use Model revision (commit hash) to use for evaluation.
  • Follow the ref doc for optional items
  • When an item is optional, don't forget to add it in the arg description. For example, for quantization config, you can add an "if needed" for ex.

src/lighteval/models/model_config.py Outdated Show resolved Hide resolved
name (str): The name of the task.
cfg (dict): The configuration dictionary containing
task-specific settings (from the task_table.json file).
cache_dir (Optional[str], optional): The directory to cache the
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the cache_dir is None

Copy link
Member Author

Choose a reason for hiding this comment

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

cache dir is not used in the class. When loading the dataset, we manually set ache_dir to None

Copy link
Member

Choose a reason for hiding this comment

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

We could remove it I guess :)

src/lighteval/tasks/lighteval_task.py Outdated Show resolved Hide resolved
src/lighteval/tasks/lighteval_task.py Outdated Show resolved Hide resolved
src/lighteval/tasks/registry.py Show resolved Hide resolved
clefourrier
clefourrier previously approved these changes Feb 6, 2024
Copy link
Member

@clefourrier clefourrier left a comment

Choose a reason for hiding this comment

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

LGTM - could be cool to be more homogeneous for optional docs but it's a very minor nit
(Optional[type] vs type, optional)

Copy link
Member

@clefourrier clefourrier left a comment

Choose a reason for hiding this comment

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

LGTM

`pretrained_model_name_or_path` argument of `from_pretrained` in the
HuggingFace `transformers` API.
accelerator (Accelerator): accelerator to use for model training.
tokenizer (Optional[str]): HuggingFace Hub tokenizer ID that will be
Copy link
Member

Choose a reason for hiding this comment

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

should be str, optional

@clefourrier clefourrier merged commit 059e100 into main Feb 6, 2024
2 checks passed
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.

2 participants