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 1.9.x into master and reapply reverted 1.9.x changes #5712

Merged
merged 24 commits into from
Apr 24, 2020

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Apr 23, 2020

Proposed changes:

  • merge 1.9.x (with reverted changes) into master
  • also re-apply the changes before merging

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):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@erohmensing erohmensing changed the title merge 1.9.x + master reapply of reverted changes merge 1.9.x into master and reapply reverted 1.9.x changes Apr 23, 2020
Copy link
Contributor

@tabergma tabergma left a 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?

Comment on lines 1275 to 1278
if self.config[NUM_TRANSFORMER_LAYERS] > 0:
# apply activation
outputs = tfa.activations.gelu(outputs)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)?

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

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 😬

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

rasa/utils/tensorflow/transformer.py Outdated Show resolved Hide resolved
changelog/5631.misc.rst Outdated Show resolved Hide resolved
@erohmensing erohmensing merged commit 32785b7 into master Apr 24, 2020
@erohmensing erohmensing deleted the 1.9.x-merge branch April 24, 2020 11:06
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

4 participants