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

Merge 2.8.x into main #9869

Merged
merged 176 commits into from
Oct 25, 2021
Merged

Merge 2.8.x into main #9869

merged 176 commits into from
Oct 25, 2021

Conversation

rasabot
Copy link
Collaborator

@rasabot rasabot commented Oct 14, 2021

🛺 This PR should be merged automatically once it has been approved. If it doesn't happen:

  • Handle merge conflicts
  • Fix build errors

💡 It has been opened automatically after changes were merged in a feature branch.

koernerfelicia and others added 30 commits September 15, 2021 10:32
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

💯 Thank you for fixing the logs! 🎉

rasa/nlu/persistor.py Outdated Show resolved Hide resolved
Co-authored-by: Tobias Wochinger <[email protected]>
@ka-bu ka-bu enabled auto-merge October 22, 2021 13:35
@ka-bu
Copy link
Contributor

ka-bu commented Oct 25, 2021

@koernerfelicia @wochinge I'm repeatedly either seeing a windows access violation error or an OOM tf error it seems on e.g. "other" for windows 3.7 or windows 3.8 -- Any idea? Did we observe this before?
There are two warnings that are just filtered out at the moment. I was assuming that those warnings appeared in 2.8.x as well - and as far as I understood the tf.gather warning should be negligible and the np.array issue didn't seem to make a difference as things where still dtype=obj (i.e. no unexpected conversions whatever happening). But if those issues are new .... 🤔

@koernerfelicia
Copy link
Contributor

Did we observe this before?

Sadly, yes, we observed this with test-nlu-predictors and windows after the TF 2.6 upgrade. The temporary fix was to run those in a single thread (as opposed to two). See here. IMO if we were okay with that temporary change, we could also do the same here, and then make an issue to follow up and revert the changes. I don't love that, but also don't have any great ideas to suggest otherwise. I know we've made some small wins to the testing suite memory consumption in the past, and Enable is planning on exploring other options (see here)

I think that the windows runners are a bit of a canary (imagery from Matt 🐤 ), in that they are the first to go OOM when we're close to the limit.

I was assuming that those warnings appeared in 2.8.x as well

I'm pretty sure you're right about that, though I'd have to check to be 100% sure.

@ka-bu ka-bu disabled auto-merge October 25, 2021 10:14
@ka-bu ka-bu enabled auto-merge October 25, 2021 12:38
@ka-bu ka-bu merged commit cba193d into main Oct 25, 2021
@ka-bu ka-bu deleted the merge-2.8.x-main-11f2118 branch October 25, 2021 15:05
@koernerfelicia
Copy link
Contributor

Congratulations, @ka-bu! What a tough one 😬 If you haven't already, can you either make an issue or extend the one I linked to remind us to revert the monothreaded testing (like this one)

@ka-bu
Copy link
Contributor

ka-bu commented Oct 25, 2021

Congratulations, @ka-bu! What a tough one 😬 If you haven't already, can you either make an issue or extend the one I linked to remind us to revert the monothreaded testing (like this one)

Thanks so much for reminding me! :) I'll extend you issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.