-
Notifications
You must be signed in to change notification settings - Fork 23
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
Initiated jobs #431
base: master
Are you sure you want to change the base?
Initiated jobs #431
Conversation
Codecov Report
@@ Coverage Diff @@
## master #431 +/- ##
=======================================
Coverage 54.03% 54.03%
=======================================
Files 38 38
Lines 12098 12099 +1
Branches 3734 3734
=======================================
+ Hits 6537 6538 +1
Misses 4579 4579
Partials 982 982
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please take a look at the comments
@@ -35,6 +35,7 @@ | |||
logger = logging.getLogger('arc') | |||
|
|||
arc_path = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) # absolute path to the ARC folder | |||
local_arc_path = os.path.join(os.getenv("HOME"), '.arc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this in https://github.com/ReactionMechanismGenerator/ARC/blob/master/arc/imports.py#L14
No need to have it here
@@ -10,7 +10,7 @@ | |||
from pprint import pformat | |||
from typing import Dict, Optional, Union | |||
|
|||
from arc.common import arc_path, get_logger | |||
from arc.common import arc_path, get_logger, local_arc_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import local_arc_path
from arc.imports
@@ -424,7 +424,7 @@ def _set_job_number(self): | |||
""" | |||
Used as the entry number in the database, as well as the job name on the server. | |||
""" | |||
csv_path = os.path.join(arc_path, 'initiated_jobs.csv') | |||
csv_path = os.path.join(local_arc_path, 'initiated_jobs.csv') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means we'll always have a .arc
folder locally. So by default, If I run from two machines on the same server, I'll get different job counters. I think this isn't the desired behaviour. Instead, I think we should only use the local arch path for the CSV files if the respective file (not folder) exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. arc is per user. If I am not mistaken if I run from two different machines I get two different job counts. I was thinking about how it working on the delta. there are multiple users using the same ARC repo. Therefore the counter is for everyone. I was afraid a permission error might occur when multiple people write to the same file. The CSV file was created automatically. Are you suggesting that we create a CSV file in the .arc folder and if the csv file exists then write to the .arc folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, check if it exists in .arc first. Other users would like to keep a songle counter while submitting from different machines onto a single server.
completed_jobs.csv and initiated_jobs.csv were located in ARC which can causes permission issues. I moved csv files to .arc folder.
found a bug in job.py; replaced "local" with server names form settings.py