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

Limit pytest jobs to 1 on CI for Windows and test-nlu-predictors #9733

Merged
merged 6 commits into from
Sep 29, 2021

Conversation

koernerfelicia
Copy link
Contributor

@koernerfelicia koernerfelicia commented Sep 28, 2021

As per discussion here, I'm limiting the pytest jobs to 1 on CI for problematic configurations (Windows + test-nlu-predictors).

This is a temporary fix in order to get TF 2.6 micro out as quickly as possible. I've created a followup issue to investigate and address the memory issues so that the fix can be reverted.

@koernerfelicia koernerfelicia marked this pull request as draft September 28, 2021 15:11
@koernerfelicia koernerfelicia marked this pull request as ready for review September 28, 2021 16:11
@koernerfelicia koernerfelicia changed the title Limit single threaded pytest to Windows and test-nlu-predictors Limit pytest jobs to 1 on CI for Windows and test-nlu-predictors Sep 28, 2021
Copy link
Contributor

@mprazz mprazz left a comment

Choose a reason for hiding this comment

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

Please rename the steps for clarity, like you said on slack , something like Test Code (monothread) vs Test Code (multithread) should suffice.

I'd also add a comment on the new step, saying it should be re-unified once memory issues are addressed.

.github/workflows/continous-integration.yml Outdated Show resolved Hide resolved
make ${{ matrix.test }}

- name: Test Code 🔍
if: needs.changes.outputs.backend == 'true' && !(matrix.python-version == 3.8 && matrix.os == 'windows-latest') && !(matrix.os == 'windows-latest' && matrix.test == 'test-nlu-predictors')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: needs.changes.outputs.backend == 'true' && !(matrix.python-version == 3.8 && matrix.os == 'windows-latest') && !(matrix.os == 'windows-latest' && matrix.test == 'test-nlu-predictors')
if: needs.changes.outputs.backend == 'true' && (matrix.python-version == 3.8 && matrix.os == 'windows-latest') && !(matrix.os == 'windows-latest' && matrix.test == 'test-nlu-predictors')

I believe you want the opposite condition here no?

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'm unsure, I was trying to keep the original behaviour: I copied this bit over from the original step and put it into both conditions because I thought we don't want this config ever to run? Or is that not the case anymore?

!(matrix.python-version == 3.8 && matrix.os == 'windows-latest')

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sorry, I missed that -- seems we're already not running win + py3.8 at all, indeed. Nevermind me, my bad ahah

koernerfelicia and others added 2 commits September 29, 2021 10:12
Co-authored-by: Mariana Prazeres <m.prazeres@rasa.com>
@koernerfelicia koernerfelicia merged commit 878d975 into tf-2.5-final Sep 29, 2021
@koernerfelicia koernerfelicia deleted the single-thread branch September 29, 2021 09:56
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.

None yet

2 participants