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

[tests] add transformers & diffusers integration tests #962

Merged
merged 5 commits into from
Sep 26, 2023

Conversation

younesbelkada
Copy link
Contributor

@younesbelkada younesbelkada commented Sep 26, 2023

Adds a workflow to manually run PEFT integration of transformers and diffusers tests to make sure new PRs will not break those integrations

cc @BenjaminBossan @pacman100

python -m pip install --upgrade pip
python -m pip install .[test]
cd .. && git clone https://github.com/huggingface/transformers.git && cd transformers/
python -m pip install ".[dev]"
Copy link

Choose a reason for hiding this comment

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

it would be great to print the transformers version (commit sha) here - it will make your life easier :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it together with printing env variables

Copy link
Member

Choose a reason for hiding this comment

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

I think Yih-Dar meant the SHA of the transformers branch, not of PEFT. It is contained in the git log but I'm not sure that it's the best way to print the hash. It's an interactive command with user input, not sure how GH actions handle those. How about git rev-parse HEAD?


jobs:
run_transformers_integration_tests:
runs-on: ubuntu-latest
Copy link

Choose a reason for hiding this comment

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

is this powerful enough to run those peft tests?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 26, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding this so quickly. It would be fine by me to merge it to main and then fix remaining issues later, as GH will only show the action once the PR is merged.

run: |
python -m pip install --upgrade pip
python -m pip install .[test]
cd .. && git clone https://github.com/huggingface/transformers.git && cd transformers/
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need an option for transformers main vs latest release too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense yes, sounds great !

@younesbelkada younesbelkada marked this pull request as ready for review September 26, 2023 09:32
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, thanks for taking this Younes. I think there are a couple of issues, please take a look.

Comment on lines 9 to 12
pytorch_nightly:
description: 'Whether to test integration tests (diffusers + transformers)'
required: false
default: false
Copy link
Member

Choose a reason for hiding this comment

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

This parameter is not being used, is it? Also, description and parameter name don't fit each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I have simply removed it here

python -m pip install --upgrade pip
python -m pip install .[test]
cd .. && git clone https://github.com/huggingface/transformers.git && cd transformers/
python -m pip install ".[dev]"
Copy link
Member

Choose a reason for hiding this comment

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

I think Yih-Dar meant the SHA of the transformers branch, not of PEFT. It is contained in the git log but I'm not sure that it's the best way to print the hash. It's an interactive command with user input, not sure how GH actions handle those. How about git rev-parse HEAD?

- name: Test transformers integration
run: |
RUN_SLOW=1 pytest tests/peft_integration/test_peft_integration.py
run_transformers_integration_tests_release:
Copy link
Member

Choose a reason for hiding this comment

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

How is it controlled that this uses the release, not the latest main? I couldn't find that in the config.

Also, there is a lot of duplication with the previous task, depending on how they differ, it might be better to turn this into a test matrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is it controlled that this uses the release, not the latest main? I couldn't find that in the config.

for run_transformers_integration_tests_release I skip the install from main part of the workflow

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, thx. So that should be pretty easy to integrate into a test matrix, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so yes, let me give it a try

@younesbelkada
Copy link
Contributor Author

Should be now ready ! Thanks a lot for your suggestions. For diffusers I have set diffusers-version: ['main'] to main only as the PEFT integration is not yet on pypi

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the changes, this LGTM.

I'd be okay with merging and then fixing any potential issues there still might be in a follow-up PR, once we can actually run the action.

@younesbelkada younesbelkada merged commit 9856f79 into huggingface:main Sep 26, 2023
@younesbelkada younesbelkada deleted the fix-peft-setter branch September 26, 2023 11:01
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.

4 participants