ReactionMechanismGenerator / AutoTST

AutoTST: A framework to perform automated transition state theory calculations
Other
32 stars 16 forks source link

This PR addresses the memory leak with dftb+ calculator on XSEDE's comet. #62

Closed skrsna closed 4 years ago

skrsna commented 4 years ago
codecov[bot] commented 4 years ago

Codecov Report

Merging #62 into master will decrease coverage by 0.07%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   62.91%   62.83%   -0.08%     
==========================================
  Files          27       27              
  Lines        4635     4601      -34     
==========================================
- Hits         2916     2891      -25     
+ Misses       1719     1710       -9     
Impacted Files Coverage Δ
autotst/conformer/systematic.py 61.20% <0.00%> (-1.94%) :arrow_down:

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 8618e28...1561e56. Read the comment docs.

nateharms commented 4 years ago

Also, I'm trying out this branch on discovery with Hotbit and I'm running into the following error:

Traceback (most recent call last):
  File "transitionstates-migration.py", line 57, in <module>
    result = job.calculate_reaction()
  File "/home/harms.n/Code/AutoTST/autotst/job/job.py", line 719, in calculate_reaction
    self.reaction.generate_conformers(ase_calculator=self.conformer_calculator)
  File "/home/harms.n/Code/AutoTST/autotst/reaction.py", line 571, in generate_conformers
    conformers = systematic_search(conformer, delta=120)
  File "/home/harms.n/Code/AutoTST/autotst/conformer/systematic.py", line 304, in systematic_search
    results = pool.map(opt_conf,tuple(to_calculate_list))
  File "/home/harms.n/anaconda2/envs/tst_env/lib/python3.7/multiprocessing/pool.py", line 268, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "/home/harms.n/anaconda2/envs/tst_env/lib/python3.7/multiprocessing/pool.py", line 657, in get
    raise self._value
  File "/home/harms.n/anaconda2/envs/tst_env/lib/python3.7/multiprocessing/pool.py", line 431, in _handle_tasks
    put(task)
  File "/home/harms.n/anaconda2/envs/tst_env/lib/python3.7/multiprocessing/connection.py", line 206, in send
    self._send_bytes(_ForkingPickler.dumps(obj))
  File "/home/harms.n/anaconda2/envs/tst_env/lib/python3.7/multiprocessing/reduction.py", line 51, in dumps
    cls(buf, protocol).dump(obj)
TypeError: cannot serialize '_io.TextIOWrapper' object
skrsna commented 4 years ago

There is no fix for that :(. I ran into the same thing when I was pickling hotbit conformer objects to insert into mongodb.

nateharms commented 4 years ago

sigh so no more hotbit for me in the future. Once I get the new AutoTST paper out, I'll move to just DFTB+ for future conformer calcs

skrsna commented 4 years ago

So, I changed the method so that multiprocessing doesn't use conformer objects as inputs or outputs but instead use the global conformers object and use index to get the conformer inside the function then run the ASE optimization and update the global conformers object. Can someone please test it with big molecules and see if the results are correct? Another way of doing this is to use delattr to delete ase_molecule attribute from conformer object but seemed unnecessary to me.

nateharms commented 4 years ago

Okay, I tried this on my local machine with Hotbit and that works perfectly fine. The coverage went down a tiny bit but by barely anything so we can let this go. Thank you for the help! I'll merge this in shortly.

skrsna commented 4 years ago

Okay, I tried this on my local machine with Hotbit and that works perfectly fine. The coverage went down a tiny bit but by barely anything so we can let this go. Thank you for the help! I'll merge this in shortly.

Please test it with large molecules on the master branch just to be sure. Thanks

nateharms commented 4 years ago

I did with a C8 molecule, it worked fine