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

Add custom JSON adapter #597

Closed
wants to merge 11 commits into from
Closed

Add custom JSON adapter #597

wants to merge 11 commits into from

Conversation

Martmists-GH
Copy link
Contributor

@Martmists-GH Martmists-GH commented Jan 17, 2017

The purpose of this adapter is to make use of different JSON parsing libraries, such as ujson or python-rapidjson.

Example usage:

chatbot = chatterbot.ChatBot(
    "Example bot",
    trainer='chatterbot.trainers.ChatterBotCorpusTrainer',
    storage_adapter={
        "import_path": "chatterbot.storage.CustomJsonFileStorageAdapter",
        "dump_func":"ujson.dumps",
        "load_func":"ujson.loads"
    }
)
chatbot.train("chatterbot.corpus.english")

As you can see, all that has to be done is to make storage_adapter a dict with an import_path, dump_func and load_func.
both dump and load functions default back to the stdlib json module. Note that dumps and loads WILL NOT WORK.

This has successfully been tested, however I do not know if I messed anything up by accident which could make it function less like it should.

Fixes #592

- save in `chatterbot.py` instead
- indents as usual
- __del__ is broken

# Successfully tested with the following code:
```py
chatbot = chatterbot.ChatBot(
"Example bot",
trainer='chatterbot.trainers.ChatterBotCorpusTrainer',
storage_adapter={
"import_path": "chatterbot.storage.CustomJsonFileStorageAdapter",
"dump_func":"ujson.dump",
"load_func":"ujson.load"
}
)
chatbot.train("chatterbot.corpus.english")
```
@Martmists-GH
Copy link
Contributor Author

@gunthercox feel free to merge whenever

Copy link
Owner

@gunthercox gunthercox left a comment

Choose a reason for hiding this comment

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

The JSON file database code is mostly contained in an external Python module (https://github.com/gunthercox/jsondb). I believe that some changes will need to happen there in order to make this work more elegantly. Ideally, the json dumps and loads methods could be set on the Database class from that library.

@Martmists-GH
Copy link
Contributor Author

Martmists-GH commented Jan 18, 2017

Noted. The reason I redid most of the I/O handling is because the current JSON adapter uses File I/O about 4 times for every filter call, causing it to slow down a lot (Not sure how much that matters on SSDs but I don't have any of those to test with). Once for every update call looked too much as well, so I set it up to save for every get_response. I will see if changing jsondb's source changes any of this, but I'm fairly sure it's the constant I/O that slows it down. At the moment even the default json.load and json.dump with the custom adapter are performing faster than jsondb, so I'll see what I can do there.

@Martmists-GH
Copy link
Contributor Author

Martmists-GH commented Jan 19, 2017

I made a PR to jsondb here. Using my PR as a test, I found that, despite it being a little faster due to it using ujson, it is still relatively slow. My guess is that the amount of File I/O calls is too high, which is why I propose to either make the amount of jsondb calls lower, or implement the custom adapter which makes less I/O calls.

@Martmists-GH
Copy link
Contributor Author

Above two commits were made to support libraries such as rapidjson, which only supports dumps and loads calls.

@vkosuri
Copy link
Collaborator

vkosuri commented Jan 21, 2017

@Martmists before pushing your commits try to runs tests your locally.

@Martmists-GH
Copy link
Contributor Author

Martmists-GH commented Jan 23, 2017

@vkosuri if you could explain to me how I will, but as said before I have no clue how to test

The weird thing with this one is that is somehow fails on py2 django 1.8 and 1.9, which I'm actually unsure of why this errors.

@vkosuri
Copy link
Collaborator

vkosuri commented Jan 23, 2017

@Martmists I think this will help you http://chatterbot.readthedocs.io/en/stable/testing.html

@Martmists-GH
Copy link
Contributor Author

I'll probably rewrite this storage adapter to use any dumping/loading method as to enable it to use for example the erlpack module.

@gunthercox
Copy link
Owner

Hi @Martmists, I apologize about the amount of time this pull request has remained open for. I'm going to close it off because I'm moving the project away from jsondb in favor of supporting popular, existing types of databases.

The JsonDatabaseAdapter in ChatterBot will be supported until the 0.7 version release but I'm not looking to add additional JSON type adapters anymore. I appreciate the work you put into this pull request and I'm sorry that I could not be merged into the project. If you still want to make it available for others to use you can always set it up as it's own Python package on PyPi.

@gunthercox gunthercox closed this Apr 29, 2017
@Martmists-GH
Copy link
Contributor Author

I see, thanks for showing interest though!
I'm going to continue using this adapter myself, and I'll leave my fork of the repo up and up to date for people who want to use it as well.

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.

3 participants