Skip to content

Conversation

@dvrogozh
Copy link
Contributor

Pushing a draft PR to check that CI will start catching #34722. If it will, I suggest to update PR #34723 with this change.

As discussed in [1], pytest-subtests changes behavior of .skipTest() having effect to really skip individual subtests or skip the entire test if module is not installed. Huggingface Accelerate has module in its dependencies. It makes sense to add it for Transformers as well to avoid divergent environment between users and ci.

See[1]: #34723 (comment)

CC: @ydshieh

As discussed in [1], pytest-subtests changes behavior of .skipTest() having
effect to really skip individual subtests or skip the entire test if module
is not installed. Huggingface Accelerate has module in its dependencies. It
makes sense to add it for Transformers as well to avoid divergent environment
between users and ci.

See[1]: huggingface#34723 (comment)
Signed-off-by: Dmitry Rogozhkin <[email protected]>
@dvrogozh
Copy link
Contributor Author

@ydshieh : I don't see that pytest-subtests propagated to circleci environment after I updated setup.py (it is not in the installed package list which circleci prints). Does circleci has different list of packages to install somewhere? or I did some mistake updating setup.py.

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 15, 2024

Hi @dvrogozh .

I think skipTest() might have some issues with pytest-subtests, see

#34723 (comment)

And I think it would be better to (for us) to figure out what happens before merging this PR.

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 19, 2024

Let's wait

pytest-dev/pytest-subtests#169

(without it, using pytest-subtests is not reliable in some cases)

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