Skip to content

Raise FileNotFoundError if sql backend tries to read a file that does not exist#1045

Merged
dwhswenson merged 5 commits into
openpathsampling:masterfrom
sroet:clear_os_error
Aug 4, 2021
Merged

Raise FileNotFoundError if sql backend tries to read a file that does not exist#1045
dwhswenson merged 5 commits into
openpathsampling:masterfrom
sroet:clear_os_error

Conversation

@sroet

@sroet sroet commented Aug 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I tried reading a file that did not exist in that directory, this lead to 2 things happening:

  1. the file was created (this made it so that it took way too long before I figured out what was going on)
  2. the code failed with:
~/github_files/openpathsampling/openpathsampling/experimental/simstore/sql_backend.py in __init__(self, filename, mode, sql_dialect, **kwargs)
    148             )
    149             engine = sql.create_engine(self.connection_uri, **self.kwargs)
--> 150             self._initialize_from_engine(engine)
    151             self.mode = mode  # in case we changed when checking existence
    152 

~/github_files/openpathsampling/openpathsampling/experimental/simstore/sql_backend.py in _initialize_from_engine(self, engine)
    154         self.engine = engine
    155         self._metadata = sql.MetaData(bind=self.engine)
--> 156         self._initialize_with_mode(self.mode)
    157 
    158     @property

~/github_files/openpathsampling/openpathsampling/experimental/simstore/sql_backend.py in _initialize_with_mode(self, mode)
    208         elif mode == "r" or mode == "a":
    209             self.metadata.reflect(self.engine)
--> 210             self.schema = self.database_schema()
    211             self.table_to_number, self.number_to_table = \
    212                     self.internal_tables_from_db()

~/github_files/openpathsampling/openpathsampling/experimental/simstore/sql_backend.py in database_schema(self)
    592             schema dictionary
    593         """
--> 594         schema_table = self.metadata.tables['schema']
    595         sel = schema_table.select()
    596         with self.engine.connect() as conn:

KeyError: 'schema'

This PR ( in 7d90a63) adds logic to raise a FileNotFound error with a test

Implementing the code and running that test led to some Warning/Errors being raised, so I fixed those in the following commits:

293c867 resolves an error my coding enviroment was throwing about self not being defined in a classmethod
3d8f488 resolves a couple DeprecationWarning: invalid escape sequence \( (or \. )inside regex strings and resolves some pep8 complaints in touched files
b8d6947 resolves SADeprecationWarning: The Engine.table_names() method is deprecated and will be removed in a future release. Please refer to Inspector.get_table_names(). (deprecated since: 1.4) in the proposed way (documentation link) and removes a leftover pytest.skip() at the end of a completed test

@sroet sroet requested a review from dwhswenson August 3, 2021 16:16
@codecov

codecov Bot commented Aug 3, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1045 (8a549c5) into master (c67edf7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1045   +/-   ##
=======================================
  Coverage   81.54%   81.54%           
=======================================
  Files         140      140           
  Lines       15400    15400           
=======================================
  Hits        12558    12558           
  Misses       2842     2842

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c67edf7...8a549c5. Read the comment docs.

dwhswenson
dwhswenson requested changes Aug 3, 2021
View reviewed changes

@dwhswenson dwhswenson left a comment

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.

Good catch! The fact that we have to do all this file handling ourselves frustrating, but it comes from the fact that most SQL databases don't really use a file model.

Tiny changes, mainly to use the SQLAlchemy as sql as in sqlbackend.py.

SQLAlchemy 1.4 made a bunch of changes. It's essentially a preview of their 2.0 while still maintaining some backward compatibility. There may be a number of other things we will have to change at some point, but they do have a clear migration guide. I think it's okay to require 1.4 even though it was just released in March, since I don't think we need to have backward compatibility for things in the experimental package.

Comment thread openpathsampling/experimental/simstore/test_sql_backend.py Outdated
Comment thread openpathsampling/experimental/simstore/test_sql_backend.py Outdated
Comment thread openpathsampling/experimental/simstore/test_sql_backend.py Outdated
@sroet @dwhswenson
Apply suggestions from code review
8a549c5 
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
@sroet

sroet commented Aug 4, 2021

Copy link
Copy Markdown
Member Author

@dwhswenson added the suggestions, please have another look.

Good catch! The fact that we have to do all this file handling ourselves frustrating, but it comes from the fact that most SQL databases don't really use a file model.

Yeah, I understand how from an sql perspective that they would just create a file if it does not exists. In this case we should deal with it to prevent users getting confused. I was mainly thrown for a loop when I did ls after getting the error and the file was actually there (it was just 0 bytes).

@sroet sroet requested a review from dwhswenson August 4, 2021 11:03
dwhswenson
dwhswenson approved these changes Aug 4, 2021
View reviewed changes

@dwhswenson dwhswenson left a comment

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.

LGTM! Merging.

Correcting my previous statement:

since I don't think we need to have backward compatibility for things in the experimental package.

Should read: "I don't think we need to support old versions of requirements for things in the experimental package." I do think that at this point we need to support backward compatibility of files as much as possible. (Files made with today's version should open in tomorrow's version.)

@dwhswenson dwhswenson merged commit dd9b5e1 into openpathsampling:master Aug 4, 2021
@dwhswenson dwhswenson added bugfix PRs fixing bugs experimental labels Aug 4, 2021
@sroet sroet deleted the clear_os_error branch August 5, 2021 10:31
This was referenced Aug 23, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwhswenson dwhswenson dwhswenson approved these changes

Assignees

No one assigned

Labels

bugfix PRs fixing bugs experimental

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sroet @dwhswenson