-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fixing CI for DM2 #12652
Fixing CI for DM2 #12652
Conversation
Still, 2 test cases fail:
|
@tmbo I had to push this commit to fix the |
🚀 A preview of the docs have been deployed at the following URL: https://12652--rasahq-docs-rasa-v2.netlify.app/docs/rasa |
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.
Just a question for now - most of it looks good, although some of the importer stuff is also so hard to understand 😅
tracker = ( | ||
await app.ctx.agent.processor.fetch_full_tracker_with_initial_session( | ||
conversation_id, | ||
output_channel=CollectingOutputChannel(), | ||
) |
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.
why is this change necessary? :)
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.
I see this is related to your comment above @m-vdb, I read the linked convo but can't tell what's the right way from that.
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.
I reverted it back to the way it was prior to changes on the dm2
branch, cf. this comment. Does it make sense?
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.
yes this change is good 👍
@@ -75,9 +75,11 @@ def test_load_from_dict( | |||
) | |||
|
|||
assert isinstance(actual, E2EImporter) | |||
assert isinstance(actual.importer, ResponsesSyncImporter) | |||
assert isinstance(actual._importer._importer, ResponsesSyncImporter) |
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.
😵💫
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.
that's one of Danil's changes. Not sure exactly why, I presume it's because some of the importer underlying code changed?
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.
ah yeah we added FlowSyncImporter
in the mix
Summary
rasa.server.py
rasa.shared
package