Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Initiated jobs #431

wants to merge 2 commits into from

Conversation

dranasinghe
Copy link

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

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #431 into master will increase coverage by 0.00%.
The diff coverage is 16.66%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
arc/job/ssh.py 20.80% <0.00%> (ø)
arc/job/job.py 21.78% <20.00%> (ø)
arc/common.py 84.04% <100.00%> (+0.03%) ⬆️

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 4577962...5dc5a71. Read the comment docs.

Copy link
Member

@alongd alongd left a 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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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
Copy link
Member

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')
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants