ReactionMechanismGenerator / AutoTST

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

[WIP] Creating a Job superclass with ThermoJob and KineticJob subclasses #68

Closed nateharms closed 2 years ago

nateharms commented 4 years ago

@davidfarinajr and I are working on PR that is going to create a superclass for Job along with subclasses for KineticJob and ThermoJob. This PR will require a version bump (technically) because it will remove backwards compatibility of previous scripts used to run AutoTST.

This PR is a WIP and is mainly being opened to allow for discussion.

nateharms commented 4 years ago

I'm working on a get_compute_requirements method to live inside a Gaussian calculator object. Thoughts on it below? @davidfarinajr, I haven't committed yet because I want to get a sense of if the requested resources are good.

    def get_compute_requirements(self, conformer=self.conformer):
        """
        A method that will take an autotst Conformer or TS object and will return
        the number of required processors that are needed and the estimated time it 
        will take to complete this

        Inputs: Conformer or TS object

        Output: tuple containing (nprocshared, memory required, time required)
        """
        number_of_atoms = conformer.rmg_molecule.get_num_atoms() - conformer.rmg_molecule.get_num_atoms('H')
        # These specifications are based on a node in the `short` partition in NEU's discovery cluster
        # These nodes typically ahve 14 cores and 120GB

        if number_of_atoms >= 4:
            nproc = 4
            mem = "30GB"
            time = "24:00:00"
        elif number_of_atoms >= 7:
            nproc = 8
            mem = "60GB"
            time = "24:00:00"
        elif number_of_atoms >= 9:
            nproc = 12
            mem = "100GB"
            time = "24:00:00"
        else:
            #essentially reserving a full node
            nproc = 14
            mem = "120GB"
            time = "24:00:00"

        return (nproc, mem, time)
rwest commented 4 years ago

I like that kind of structure. Then in job.py it can call it and figure out what to do with that requirement.

codecov[bot] commented 4 years ago

Codecov Report

Merging #68 into master will decrease coverage by 12.30%. The diff coverage is 16.93%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #68       +/-   ##
===========================================
- Coverage   64.12%   51.81%   -12.31%     
===========================================
  Files          27       29        +2     
  Lines        4621     5398      +777     
===========================================
- Hits         2963     2797      -166     
- Misses       1658     2601      +943     
Impacted Files Coverage Δ
autotst/job/job.py 14.50% <4.34%> (-29.60%) :arrow_down:
autotst/job/thermo.py 5.54% <5.54%> (ø)
autotst/job/kinetics.py 13.75% <13.75%> (ø)
autotst/calculator/gaussian.py 48.25% <41.17%> (-14.98%) :arrow_down:
autotst/calculator/orca.py 73.88% <68.14%> (-9.63%) :arrow_down:
autotst/calculator/orca_test.py 93.75% <94.44%> (-3.31%) :arrow_down:
autotst/job/job_test.py 32.98% <0.00%> (-65.98%) :arrow_down:
autotst/calculator/gaussian_test.py 87.93% <0.00%> (-6.90%) :arrow_down:
... and 6 more

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 5bf96a6...acc9899. Read the comment docs.

nateharms commented 4 years ago

I was maybe thinking that this would be called by a Gaussian calculator when it was creating and ASE Gaussian object and would then attach all of this info to the ase calculator which would then be used by Thermo- or KineticJob to run the slurm submits. But the idea of Thermo- or KineticJob calling it itself makes sense too. 🤔

davidfarinajr commented 4 years ago

I'm working on a get_compute_requirements method to live inside a Gaussian calculator object. Thoughts on it below? @davidfarinajr, I haven't committed yet because I want to get a sense of if the requested resources are good.

    def get_compute_requirements(self, conformer=self.conformer):
        """
        A method that will take an autotst Conformer or TS object and will return
        the number of required processors that are needed and the estimated time it 
        will take to complete this

        Inputs: Conformer or TS object

        Output: tuple containing (nprocshared, memory required, time required)
        """
        number_of_atoms = conformer.rmg_molecule.get_num_atoms() - conformer.rmg_molecule.get_num_atoms('H')
        # These specifications are based on a node in the `short` partition in NEU's discovery cluster
        # These nodes typically ahve 14 cores and 120GB

        if number_of_atoms >= 4:
            nproc = 4
            mem = "30GB"
            time = "24:00:00"
        elif number_of_atoms >= 7:
            nproc = 8
            mem = "60GB"
            time = "24:00:00"
        elif number_of_atoms >= 9:
            nproc = 12
            mem = "100GB"
            time = "24:00:00"
        else:
            #essentially reserving a full node
            nproc = 14
            mem = "120GB"
            time = "24:00:00"

        return (nproc, mem, time)

I think this should be within each method of the gaussian calculator because each method will have different requirements. For example, I have different requirements for get_conformer_calc, which runs DFT optimization, and get_sp_calc which I use to run G4 method. Single point methods require more mem than DFT. Also, I use more procs for the rotor calculator because that takes longer.

nateharms commented 4 years ago

Okay, that makes sense. Would you mind tackling that for the Gaussian calculator then? I guess I was trying to make a broad way of determining required compute resources, but it's seeming like it needs to be tailored to each specific opt type.

nateharms commented 4 years ago

rebased this and I'm working on the fstrings for job/thermo and job/kinetics at the moment

davidfarinajr commented 4 years ago

I added nproc/mem/time to gaussian calculator methods with latest commit. Also, since we need to run freq jobs separately, I modified the get_conformer_calc so it can also generate freq only input files. @nateharms, can you take a look at this when you get a chance. We should also discuss the scaling of the computational resources.

skrsna commented 4 years ago

May I suggest having a dictionary of specs for each cluster so that we can switch between NEU or Comet without having to change the source code. We can do this automatically calling sacctmgr show Cluster format=cluster using subprocess and parsing the output.