-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 1.9.x into master and reapply reverted 1.9.x changes #5712
Conversation
make timeout for rasa shell configurable
Co-Authored-By: Akela Drissner-Schmid <32450038+akelad@users.noreply.github.com>
Revert "Move activation to transformer block"
add whatsapp+ prefix to twilio creds
Revert model breaking changes in 1.9.x
prepared release of version 1.9.7
Co-Authored-By: Tanja <tabergma@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments. What about the changes from #5565? I guess we don't see any of those changes here, because those changes are already on master and you removed the changes that would have removed them, correct?
if self.config[NUM_TRANSFORMER_LAYERS] > 0: | ||
# apply activation | ||
outputs = tfa.activations.gelu(outputs) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep this if we want to reapply the changes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I thought so. Not sure what went wrong, i'll try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait sorry i should clarify -- this PR was supposed to already reapply the changes. Is it possible that the activation PR wasn't ever merged into master (but genie's were)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be strange... but maybe there were some merge conflicts and someone did not resolve them properly...
Just checked master. The changes over there are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've tried a few different things with the commits and haven't gotten it to play nice. 5565 is correct on master and no diff, and the right side here looks correct (even if the diff doesn't correctly reflect master). What if we merge, then check master to make sure everything is how we expect? I think the end result of this PR is correct...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may make more sense for you to take over this merge/re-apply since i don't know intuitively which changes are supposed to go where 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry. We first moved it but then decided to keep it where it was and wrap it with an if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok let's definitely rename that pr 🙈 i think i missed a few commits, let me try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated everything. Looks good now I would say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you! I think i just missed 2 of your commits which made stuff weird. renamed the PR.
Proposed changes:
it ended up being easier to do both in the same PR, as we'd just have to resolve the conflicts and then re-resolve them if we did them in 2.
Status (please check what you already did):
black
(please check Readme for instructions)