Skip to content

Switch to SQLAlchemy for the Recorder component. Gives the ability t…#2377

Merged
balloob merged 7 commits into
home-assistant:devfrom
rhooper:sqlalchemy
Jul 2, 2016
Merged

Switch to SQLAlchemy for the Recorder component. Gives the ability t…#2377
balloob merged 7 commits into
home-assistant:devfrom
rhooper:sqlalchemy

Conversation

@rhooper
Copy link
Copy Markdown
Contributor

@rhooper rhooper commented Jun 26, 2016

Description: Adds SQLAlchemy support.

Major change: This now uses homeassistant2.db as the default DB because of shift in data types for timestamps.

Example entry for configuration.yaml (if applicable):

recorder:
  db_url: mysql://localhost/homeassistant

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

return []


def row_to_state(row):
Copy link
Copy Markdown
Member

@balloob balloob Jun 27, 2016

Choose a reason for hiding this comment

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

Should these be moved to be (a static) methods on the models?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably should do so. I wasn't ever very happy with these, but they mimic the old structure. Now's the time to clean up though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the end, I decided to refactor this to rely on model methods to convert to native form and an execution helper.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 27, 2016

This is awesome! I've only had a few minor comments.

if not db_url:
db_url = DEFAULT_URL.format(
hass_config_path=hass.config.path(DEFAULT_DB_FILE))

Copy link
Copy Markdown
Member

@balloob balloob Jun 27, 2016

Choose a reason for hiding this comment

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

This comment is not perse related to this place in the code but GitHub otherwise has no threaded conversations.

One problem we have with current conversion script is that the script directory is not shipped to pypi. I think it would make sense if we create a new package homeassistant.scripts that contains scripts we want to bundle with HA or just put it in the recorder package for now.

I wonder if we would want to do the migration automatically. Should we add here something like this:

import sys
from subprocess import check_output
from homeassistant.components.recorder import convert_db_to_sqlalchemy

old_path = hass.config.path('home-assistant.db')
if os.path.isfile(old_path):
    check_output([sys.executable, convert_db_to_sqlalchemy.__file__, hass.config.config_dir, db_url])
    os.remove(old_path)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Running the migration automatically could take a very long time, may run us out of disk space, or may wear out SD cards. We could migrate the current day's data automatically, though?
With regards to moving the script into homeassistant.scripts, that's easy and lets us adapt the conversion script for automation if needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved the script as suggested.

Comment thread .coveragerc Outdated
homeassistant/components/upnp.py
homeassistant/components/zeroconf.py

homeassistant/scripts/*.py
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you move this to be under the line __main__

@balloob balloob merged commit a2e45b8 into home-assistant:dev Jul 2, 2016
@nkgilley
Copy link
Copy Markdown
Contributor

How do we supply username and password for mysql? I tried this and didn't get any errors but the db is still empty:

recorder:
  db_url: mysql://192.168.1.50/homeassistant?user=root&password=pw

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 14, 2016

Search for sql alchemy connection string.. We should make sure we have
MySQL instructions in recorder docs before release

On Thu, Jul 14, 2016, 07:58 Nolan Gilley notifications@github.com wrote:

How do we supply username and password for mysql? I tried this and didn't
get any errors but the db is still empty:

recorder:
db_url: mysql://192.168.1.50/homeassistant?user=root&password=pw


You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#2377 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABYJ2mFP25g4w2_Wak3XL4jTQuRUjidiks5qVk6AgaJpZM4I-o9q
.

@rhooper
Copy link
Copy Markdown
Contributor Author

rhooper commented Jul 14, 2016

Try this:

recorder:
   db_url: mysql://user:password@serverip/dbname

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 14, 2016

@rhooper you think we should auto install support package based on database
adapter they want to use?

If protocol == MySQL, install mysqlclient etc

On Thu, Jul 14, 2016, 09:10 rhooper notifications@github.com wrote:

Try this:

db_url: mysql://user:password@serverip/dbname


You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#2377 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABYJ2vDGFp3_loE3Cc6WcLDO4PEeKu1rks5qVl94gaJpZM4I-o9q
.

@rhooper
Copy link
Copy Markdown
Contributor Author

rhooper commented Jul 14, 2016

If that's easy to implement, yes. Otherwise we'll have a lot of confused users.

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Jul 14, 2016

This commit contains a couple of db_urls. I don't think that I make sense to add Oracle or Firebase but perhaps Microsoft SQL Server for the Windows users. Is mssql+pyodbc://username:password@db_name a default URL for Microsoft SQL Server?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 14, 2016

I don't have time for this but these snippets should help:

>>> from urllib.parse import urlparse
>>> urlparse('postgresql://scott:tiger@SERVER_IP/DB_NAME')
ParseResult(scheme='postgresql', netloc='scott:tiger@SERVER_IP', path='/DB_NAME', params='', query='', fragment='')
import homeassistant.util.package as pkg_util

pkg_util.install_package('mysqlclient', target=hass.config.path('deps'))

@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants