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

updated dbprocessing code for sqlalchemy 2.0 #137

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ezd1000
Copy link
Collaborator

@ezd1000 ezd1000 commented Feb 2, 2024

Updated a series of errors so that dbprocessing code works for both sqlalchemy 2.0 and sqlalchemy 1.4. Closes #135.

PR Checklist

  • Pull request has descriptive title
  • Pull request gives overview of changes
  • (N/A) New code has inline comments where necessary
  • (N/A) Any new modules, functions or classes have docstrings consistent with dbprocessing style
  • (N/A) Major new functionality has appropriate Sphinx documentation
  • (N/A) Added an entry to release notes if fixing a major bug or providing a major new feature
  • (See below) New features and bug fixes should have unit tests
  • Relevant issues are linked in the description (use Closes # if this PR closes the issue, or some other reference, such as See # if it is related in some other way)

*We don't yet have unit tests to make sure warnings aren't raised, the tests now pass without warnings on Python 3.10, sqlalchemy 1.4, and 2.0.

@jtniehof
Copy link
Member

jtniehof commented Feb 6, 2024

This would close #135, so should edit the PR description to say that. (Last item in the PR checklist, see #120 for an example, also some information here.)

Copy link
Member

@jtniehof jtniehof left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Just to make history less arduous, can you do some rebasing and squash some commits together?

  • the four commits, three "added bind to drop_all" and "removed self from dbu_engine" should be one
  • "fixed warning message" and "fixed all warning messages" should be one
  • the "added bind=self.engine" and "took out bind=self.engine" should be combined; really this is "move the bind from Metadata constructor to table creation"
  • probably the two "fixed another join" should be one

Copy link
Member

@jtniehof jtniehof left a comment

Choose a reason for hiding this comment

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

I did a few quick checks for places which aren't in the unit tests but show a similar pattern to what we had to change.

create_all requiring the bind now: CreateDBsabrs line 321 (@dnadeau-lanl , is CreateDBsabrs now out of date, or is it superseded now that we can create postgres databases? Maybe we should be removing it instead.)

execute no longer taking a direct SQL string: scrubber line 27, clean_test_db line 119. To test the scrubber, we're actually kind of short on canned test databases (the psp one is enormous now). You can get an old one from history by git show e3c67f3feabdc62bec2ffabba78bceb3d8dd5d89:./functional_test/testDB.sqlite > foo.sqlite. You can get the old mageis database for testing clean_test_db with git show 18469404b10ece57051b9b2391c1ea3c0741aefe:./unit_tests/RBSP_MAGEIS.sqlite > RBSP_MAGEIS.sqlite

MetaData no longer taking a bind argument (having to bind separately): CreateDBsabrs line 40 (like in other places where we've made that change, that line needs to be fixed and specifying the bind may have to happen in multiple places) EDIT: bind needs to be added to the reflect call in init_db (right after it was removed from the instantiation of self.metadata, and somewhat related the create_all call around line 321 already has the bind, but should be self.engine not engine.

The functional test is also not running. Unfortunately it doesn't run in the CI (#68) so it's possible this has just drifted out of date. Running it is pretty simple, run python ./functionalTest.py in the functional_test directory. Unfortunately debugging it is going to be harder; it makes a temp directory where all the scripts and such live, and for some reason when the codes fail to run, the moveToError that copies the error log to the error directory is also failing.

For finding these various scripts, use find in the dbprocessing checkout.

dbprocessing/DButils.py Outdated Show resolved Hide resolved
docs/CONTRIBUTORS.rst Outdated Show resolved Hide resolved
scripts/scrubber.py Outdated Show resolved Hide resolved
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.

Port dbp to SQLAlchemy 1.4/2.0
2 participants