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

Add linting capability #52

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

hickeyma
Copy link
Collaborator

@hickeyma hickeyma commented Feb 21, 2024

Added the following linter:

Note to reviewers:

  • Added tox target which can kick off linting.
  • Separated packages that require CUDA into cuda_requirements.txt as they are not required for linting and also are not required for non CUDA tuning.
  • The linting is not automated yet as need to add CI/CD
  • Any changes recommended by the linter are not committed until we automate as part of the build
  • The linting can be run manually:
    • Installing tox and dependencies: pip install -r setup_requirements.txt
    • Running it: tox -e lint

Signed-off-by: Martin Hickey <[email protected]>
Copy link
Collaborator

@anhuong anhuong left a comment

Choose a reason for hiding this comment

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

A few questions Martin

  • Could you add how to run the linter and formatter in the README? Especially if this is something that is expected to be run by all contributors else PRs will fail (in near term future when automation added)
  • If we separate torch_requirements, this should also be updated in the README since it will require users to run pip install -r torch_requirements.txt and pip install -r requirements.txt. Which wil affect any current automation that is installing this repo.

.pylintrc Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
hickeyma added a commit to hickeyma/fms-hf-tuning that referenced this pull request Feb 23, 2024
Review comments:

- foundation-model-stack#52 (review)

Signed-off-by: Martin Hickey <[email protected]>
@hickeyma
Copy link
Collaborator Author

@anhuong Thanks for the review and feedback.

Could you add how to run the linter and formatter in the README? Especially if this is something that is expected to be run by all contributors else PRs will fail (in near term future when automation added)

I intend to do that in the contributors doc as part of the enablement of linting in CI/CD. Next PR essentially.

If we separate torch_requirements, this should also be updated in the README since it will require users to run pip install -r torch_requirements.txt and pip install -r requirements.txt. Which wil affect any current automation that is installing this repo.

Added to the README now.

@hickeyma
Copy link
Collaborator Author

@anhuong Updated and ready for review again.

@hickeyma hickeyma requested a review from anhuong February 23, 2024 18:56
@anhuong
Copy link
Collaborator

anhuong commented Feb 26, 2024

I tried uninstalling packages, wheel, and flash-attn and running tuning to make sure it can run without these deps.

When trying to run PT, I get error:

Traceback (most recent call last):
  File "fms-hf-tuning/tuning/sft_trainer.py", line 8, in <module>
    from peft.utils.other import fsdp_auto_wrap_policy
  File "fms-hf-tuning/venv/lib/python3.9/site-packages/peft/__init__.py", line 22, in <module>
    from .auto import (
  File "fms-hf-tuning/venv/lib/python3.9/site-packages/peft/auto.py", line 23, in <module>
    from transformers import (
  File "fms-hf-tuning/venv/lib/python3.9/site-packages/transformers/__init__.py", line 26, in <module>
    from . import dependency_versions_check
  File "fms-hf-tuning/venv/lib/python3.9/site-packages/transformers/dependency_versions_check.py", line 16, in <module>
    from .utils.versions import require_version, require_version_core
  File "fms-hf-tuning/venv/lib/python3.9/site-packages/transformers/utils/__init__.py", line 19, in <module>
    from packaging import version
ModuleNotFoundError: No module named 'packaging'

This is a package that is usually installed with flash-attn I believe, so if flash-attn is not installed this may need to be added to core requirements.

These are the other packages that rely on packaging:

$ pip show packaging
Name: packaging
Version: 23.2
Summary: Core utilities for Python packages
Home-page: 
Author: 
Author-email: Donald Stufft <[email protected]>
License: 
Requires: 
Required-by: accelerate, aim, datasets, huggingface-hub, peft, transformers

Pip installing packaging resolves the above error and PT runs successfully.

@anhuong
Copy link
Collaborator

anhuong commented Feb 26, 2024

Can verify that with set of optional deps uninstalled and with packaging installed, Lora tuning also runs successfully.

Review comments:

- foundation-model-stack#52 (review)

Signed-off-by: Martin Hickey <[email protected]>
@hickeyma
Copy link
Collaborator Author

As discussed @anhuong, I have moved packaging to the core requirements file.

Copy link
Collaborator

@anhuong anhuong left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Martin!

@anhuong anhuong merged commit f4e8eb4 into foundation-model-stack:main Feb 26, 2024
2 checks passed
@hickeyma hickeyma deleted the chore/add-linting branch February 26, 2024 20:49
@tedhtchang
Copy link
Collaborator

We should enable fmt and lint checks for each PR #59

anhuong pushed a commit to anhuong/fms-hf-tuning that referenced this pull request Apr 3, 2024
Review comments:

- foundation-model-stack#52 (review)

Signed-off-by: Martin Hickey <[email protected]>
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