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

Remove outdated backwards compat code #9187

Merged
merged 10 commits into from Jul 28, 2021
Merged

Remove outdated backwards compat code #9187

merged 10 commits into from Jul 28, 2021

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Jul 23, 2021

Fixes #6487

Proposed changes:

  • The code changed in this PR was introduced for backwards compat reason, but has since evolved. In all the cases I think we can get rid of the various behaviours introduced (argumenting on each of them in the PR to make the review easier)

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)

@m-vdb m-vdb requested a review from a team as a code owner July 23, 2021 08:06
rasa/cli/x.py Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Jul 26, 2021

I need to figure out why tests are currently failing

@wochinge
Copy link
Contributor

@m-vdb Feel free to request review from me then 👍🏻 (instead of the entire squad)

@wochinge wochinge requested review from wochinge and removed request for a team July 26, 2021 13:02
@m-vdb
Copy link
Collaborator Author

m-vdb commented Jul 26, 2021

@wochinge after some search I think I understand why the tests are currently failing, but I don't know how to fix this yet. Maybe you have an idea. The different event instances should have a metadata attribute, but for some reason, after they're deserialised via jsonpickle, this doesn't work (hence those attribute errors). I suspect some sort of cache in jsonpickle. But I'm not quite sure here 🤔 does this sound familiar?

These tests are failing, and at line 149 we deserialise events through a helper from conftest.

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.

Re the failing tests:

  1. The old pickled dialogue files don't have the metadata property
  2. Jsonpickle reloads the pickled dialogue without using the constructor (e.g. here, "When a class instance is unpickled, its init() method is usually not invoked.")
  3. There is no attribute metadata as we have never assigned this attribute in the first place

As far as I can see from the events and tracker_store modules we are never restoring a tracker using pickle or jsonpickle which in turn means that the jsonpickling is no functionality which we need to test. You could

  • re-dump the tracker states via json.dumps(dialogue.as_dict()) and then use json.loads to the load the tracker and execute the (actual) test.
  • drop the persisted dialogues and inline them as list of events in the code. This way they are closer to the test code and we don't use code to serialize / deserialize trackers which we don't use anywhere else (I think I prefer this one)

rasa/cli/x.py Show resolved Hide resolved
tests/test_model_training.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Jul 27, 2021

@wochinge I've addressed your comment regarding test data in this commit: 12262cd. One thing that I wasn't sure about: where to put the test data. I've moved it in conftest.py as other tests depend on it (as you'll see in the commit).

@m-vdb m-vdb requested a review from wochinge July 27, 2021 16:30
@m-vdb m-vdb force-pushed the make-deprecations-explicit branch from f6c1026 to 210d88c Compare July 27, 2021 16:34
@m-vdb m-vdb force-pushed the make-deprecations-explicit branch from 210d88c to 12262cd Compare July 27, 2021 16:38
tests/conftest.py Outdated Show resolved Hide resolved
@m-vdb m-vdb enabled auto-merge July 28, 2021 09:57
@m-vdb m-vdb merged commit 071d2ef into main Jul 28, 2021
@m-vdb m-vdb deleted the make-deprecations-explicit branch July 28, 2021 10:22
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.

Raise proper user warnings for backwards compatibility code
2 participants