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

conform db access using ego.io #16

Closed
nesnoj opened this issue Feb 8, 2017 · 15 comments
Closed

conform db access using ego.io #16

nesnoj opened this issue Feb 8, 2017 · 15 comments

Comments

@nesnoj
Copy link
Member

nesnoj commented Feb 8, 2017

The original post was openego/data_processing#13, further discussions can be found in #14 (comment):

In either case we might aspire to bundle db access stuff in ego.io tools - currently db access is done using

  1. oemof.db import statements (e.g. here)
  2. data_proc io (e.g. here)
  3. ego.powerflow tools (e.g. here)

We could move the oedb_session() function to ego.io and adjust all scripts in all repos to use it..?

This is on schedule for v0.3.

@nesnoj nesnoj added this to the Release 0.3 milestone Feb 8, 2017
@nesnoj
Copy link
Member Author

nesnoj commented Aug 29, 2017

Done in #34

@nesnoj nesnoj closed this as completed Aug 29, 2017
@mariusves
Copy link
Contributor

I tried to use this connection provider with eTraGo so we could scrap the sessionmaker there.
Unfortunately the function oemof_connection() failed to retrieve my password from the keyring which made me enter it every time (maybe a windows issue?). Furthermore, the connection provider produces an error if a certain servername is not in the config file. I've made some changes to our sessionmaker here. It also includes a config-reader function similar to oemof.db, even though the password is not stored in the keyring but in the config file itself.

I'd propose the following "flow" for the connection provider:

  1. Section (or servername) is present in config --> use config file
  2. Section (or servername) is not present in config --> Ask if new connection details should be added to config or use them only once without saving
  3. Config file not present --> use current implementation of create_oedb_config_file()

@nesnoj @gplssm @ulfmueller
Any feedback on this? I would go ahead and implement the changes in a new branch to make it a bit clearer what my idea for this is.

@nesnoj
Copy link
Member Author

nesnoj commented Nov 24, 2017

I tried to use this connection provider with eTraGo so we could scrap the sessionmaker there.
Unfortunately the function oemof_connection() failed to retrieve my password from the keyring which made me enter it every time (maybe a windows issue?)

I do not know, it works well on Linux. Comments from windoze users, anyone?

I'd propose the following "flow" for the connection provider:

  1. Section (or servername) is present in config --> use config file
  2. Section (or servername) is not present in config --> Ask if new connection details should be added to config or use them only once without saving
  3. Config file not present --> use current implementation of create_oedb_config_file()

This sounds reasonable, please go ahead 👍 . But could you please implement it in ego.io (not eTraGo) and create a pull request? If you like, I could review the changes..
I propose to move the part with the manual input to a separate function and use it during creation of a new file as well as during adding a new section.

Do you use egoio.tools.db.connection() in eTraGo or do you import directly from oemof?
(see example in eDisGo).

@mariusves
Copy link
Contributor

With commit 8311a20 I implemented my idea of a new config reader/writer.
image

Please feel free to share your feedback!

@gplssm
Copy link
Contributor

gplssm commented Nov 27, 2017

Thx for your effort! I would like to overcome the dependency of oemof.db with its archaic remnants just to initiate a DB connection ;-)

I haven't tried your code and I won't do it this week. But I'm highly interested to use it with ding0 and eDisGo!

If someone of you thinks about how can I test all relevant use case?, here's a list of things I would try if they work

  1. Reading from a new section in the default config.ini (file already exists)
  2. Reading from a new section in the default config.ini (file does not exist)
  3. Reading from a new section in the non-default config.ini (use an arbitrary location, specified by the user). Write user input to that file
  4. (optional) First check if connection details provided by input from prompt are valid and only save them if they are valid.

@nesnoj
Copy link
Member Author

nesnoj commented Nov 28, 2017

Thanks @mariusves for improving the db stuff, I'll test it within this week..

I would like to overcome the dependency of oemof.db with its archaic remnants just to initiate a DB connection ;-)

I fully agree, time to get rid of it!

@gplssm
Copy link
Contributor

gplssm commented Dec 8, 2017

Another note: currently when I use ego.io to create a database connection for the first time and enter the input for the config file, the password that I enter to be stored in the keyring in given plain text. Please adapt the password input to used getpass.

@nesnoj
Copy link
Member Author

nesnoj commented Dec 8, 2017

Yupp, same here. But getpass is already used, it says that it fall backs to plaintext for some reason..

@mariusves
Copy link
Contributor

I've noticed that, too. Are you using Spyder/QTconsole, too?
When searching for this issue I found the following issue here saying it was fixed in Spyder 2.3.9 & Qtconsole 4.2.1. Even though I'm using newer versions, this error still persists on my machine (win10 64bit).

@nesnoj
Copy link
Member Author

nesnoj commented Dec 8, 2017

W're both using pyCharm

@nesnoj
Copy link
Member Author

nesnoj commented Dec 8, 2017

@mariusves I've ran your version last week but got an error, so I decided not to dig deeper - this is what @gplssm mentioned in #41.
Now I reviewed your changes. In the end, the reason was a differently named key (db instead of database) - now it works.

But I fixed some small issues:

  1. Rename db to database (more user-friendly from my pov)
  2. Bugfix: Check if pass is None after retrieval of pass from keyring (in your version, u used try..except but if the password does not exist, keyring returns None instead of raising an exception!)
  3. Removed oemof.db from setup.py, added keyring

I approved @gplssm's cases 1.-3. from above.

Merged into dev.
Thanks for your contribution!

@nesnoj
Copy link
Member Author

nesnoj commented Dec 8, 2017

If you're fine with this, please close @mariusves

@mariusves
Copy link
Contributor

Thanks for the fixes and merging!

@ulfmueller
Copy link
Member

ulfmueller commented Dec 11, 2017

Nice! Therefore eTraGo, eGo, eDisGo, dp (...) can (should) all now use this ego.io db connector?!
I suppose in each app are some (minor) adjustments necessary which all the individual app teams will manage, correct?!

@mariusves
Copy link
Contributor

I can push the update to the new connector for eTraGo (already working with it locally).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants