Skip to content

Db config#37

Open
frobnitzem wants to merge 2 commits intoHPCL:mainfrom
frobnitzem:db_config
Open

Db config#37
frobnitzem wants to merge 2 commits intoHPCL:mainfrom
frobnitzem:db_config

Conversation

@frobnitzem
Copy link

These changes to the Fetcher class allow database connection information (host, port, user, password, database name) to be configured via environment variables. Ideally, these could all be set in URL form via a single environment variable, e.g.
mysql://scott:tiger@localhost:5432/mydatabase, such as provided by SQLAlchemy.

Perhaps mysql can be replaced with sqlalchemy?

Note also that package requirements have been added to setup.py so that a single pip install uo-tools will also add dependencies.

Note also that there are some other places where this change needs to be made in order to be effective throughout the project (notably in the DbInterface command-line!).

@fickas
Copy link
Contributor

fickas commented Mar 16, 2023

Hi David

Thanks for trying things out. I have a couple general comments/questions. I'll let Jason add comments about code specifics.

  1. Can you move your comments in your readme.me from CAT-SDK here to make it a little clearer what this PR is about? Maybe add an Issue with some of that text? And link this PR to it.

  2. I'm not sure what the overall goal is. Do you want to dockerize (a) the DB, (b) a jupyter server and (c) a clone of the repo? So from this one docker image I can run the notebooks from gremcat?

@jprideaux
Copy link
Contributor

Hi David,
Were you able to get db_interface to work? On our servers, we run that command from just outside the src folder where it sits. Perhaps you're missing some package that it relies on, though. Feel free to send me any specific errors that you are getting.

@frobnitzem
Copy link
Author

The docker compose setup here illustrates the goal: https://github.com/CAT-SDK/gremcat-docker

I want to run both the database and the jupyter notebook server within containers so that installation is simple, modular, and self-contained. Docker compose provides that -one docker container runs a mysql database, and another runs the web server.

However, there are some packaging issues with ideas-uo that make this setup incomplete. First, ideas-uo is hard-coded in several places to work only with a specific server (sansa.uoregon.edu?). This PR allows the Fetcher class to use a different server, depending on the environment variables. It's not complete, however, since db_interface uses yet another method to configure the database (via command-line). Ideally, there would be one convention. I'd prefer an alembic URL in an environment variable.

Second, unrelated to this PR, the db_interface is not configured to work as a packaged script. It uses relative imports including src/..., which requires cloning this repo and running python from the root directory. The established python convention is to use relative imports from the script's current directory. This way, I can run python setup.py install, then import db_interface.

@jprideaux
Copy link
Contributor

Ok, I see no problem changing those imports in the python files in gitutils to be relative. I tried it out on our arya server, and it everything worked fine.

@jprideaux
Copy link
Contributor

jprideaux commented Apr 3, 2023

I've tried to change to relative imports, but that now seems to be breaking outside tools that rely on our gitutils package (such as Meercat). Often we set the pythonpath env variable to the folder that contains src/gitutils/ and other tools before we use db_interface or update_database, so I wonder if that would work for you? I'm open to trying other options as well.

I'm taking another stab it, and I might have it figured out (just need to test out the changes with our other scripts, notebooks, and meercat. If I do get it working, should I just add the changes to this db_config branch? Thanks.

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