Added a raise DatabaseError to thermo.py

[amarkpayne]

to avoid infinite looping incase a thermo pointer is corrupt

[amarkpayne]

I was looking at a long job that I was running on the RMG server, and I noticed that the model generation had not advanced over the course of 15 days. I know that @cgrambow has also had issues with RMG jobs becoming hung-up. I have no idea what is causing these two cases, but it made me remember that @alongd had experienced an issue like this, and had made the commit above to remedy the situation.

This commit was originally on #1039, but I believe that merging this commit is urgent enough to be its own pull request. This might not be the cause of the other hang-ups, but I think it is best to fix this possibility either way.

[cgrambow]

I agree, merging the commit probably can't hurt, but I'm not sure it's the cause of the current hang-ups. My jobs became hung-up after about two days on the RMG server, but they have been running without issue on Pharos for four days now. It may be an issue of SLURM losing communication with jobs.

[mjohnson541]

To my understanding the while loop above should only execute once, so shouldn't it be an if statement instead?

[mjohnson541]

Also there are two ways for the while loop above it to terminate:

1. node = None
   or
   2 ) node.data has something that isn't None
   in case 1 it will error when it sets data = node.data

so to reach this while loop we have to start with data is not None

since the while loop only executes once we don't need to check that data is not None in the if statement

[codecov]

Codecov Report

Merging #1180 into master will increase coverage by <.01%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   46.35%   46.35%   +<.01%     
==========================================
  Files         151      151              
  Lines       27548    27554       +6     
  Branches     5406     5407       +1     
==========================================
+ Hits        12769    12772       +3     
- Misses      13896    13898       +2     
- Partials      883      884       +1

Impacted Files	Coverage Δ
rmgpy/data/thermo.py	74.63% <55.55%> (-0.16%)	⬇️

---

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 6c8976f...a408978. Read the comment docs.

[amarkpayne]

Updated with suggestions from @mjohnson541 to make the code more logical. Thanks for pointing out that the 'while' statement was unnecessary since the code has failed if the loop is run more than once.

[mliu49]

I think @mjohnson541 interpreted the code incorrectly. The while loop ensures that we continue searching until we reach a node that has actual data. Here's a pseudocode interpretation:

while data is a string and not None
    search through all the entries 
        if we find an entry that has a label which matches data
            set data to the new entry's data
            stop searching
    (implicit: if the data is still a string, restart the while loop; if the data is actual thermo data or None, exit the while loop)

If none of the entries match the label that it is searching for, the while loop will never exit. It is also possible that there is a circular reference in the thermo entries, which would also prevent the while loop from exiting. This seems to rely heavily on the assumption that the database is properly written...

[mjohnson541]

Oh, sorry I wasn't aware the thermo database had references that way.

[amarkpayne]

@mliu49 is the 'and not None' necessary in the while statement? If data is a string, can it be None?

[mliu49]

I think this looks good. I agree that and not None is not necessary.

I'm wondering if it's necessary to put an iteration limit on the while loop to catch cases with circular references, or if we trust our ability to prevent such cases from being added to the database.

[mjohnson541]

Given how difficult it can be to track some of these issues I certainly think that would be worthwhile.

[amarkpayne]

Loop counter added as requested.

[mliu49]

Could you change the error message to "Maximum iterations reached while following thermo group data pointers. A circular reference may exist. Last node was ..."

[amarkpayne]

Error message updated.

[mliu49]

I think this is ready to merge.

[amarkpayne]

All I did is rebase, should this actually require another review? Or did I mess something up? I might also see if I have access to approve this request (I am only doing this since Max approved this pull request, I won't approve my own pull requests in the future).

[amarkpayne]

Nevermind, Github won't let authors approve their own pull request. Makes sense

[mliu49]

The settings now require an approved review before merging, but if you update the branch, prior approvals get removed. This can be annoying if you only rebased, but is intended to prevent approvals from carrying over if new changes are made.

I'll approve and merge this once Travis finishes.