Acquisition change for run in cronicle #1108
Conversation
…l problem: This change allows the test to pass, but doesn't accurately test what happens when indicator_name is not included as argument when acquisition is run. Should probably use an actual argparse object for the args instead of a MagicMock to test next commit.
There was a problem hiding this comment.
Looks good! I will give my 👍 , but I think that @krivard and/or @melange396 should put eyes on it as well.
| help="filename for log output (defaults to stdout)") | ||
| parser.add_argument( | ||
| '--indicator_name', | ||
| nargs='?', |
There was a problem hiding this comment.
whats the purpose of this option? do we need it?
There was a problem hiding this comment.
@melange396 It's to run acquisition on one specific indicator. We do need it.
A bit of context: the final goal is to run acquisition immediately after each specific indicator run on Cronicle, so the database gets csv files immediately when new data is available.
Right now on Automation, acquisition runs at a specific time, on any indicators with new csv available at the time. Since acquisition process checks all indicator directories under data_dir/receiving, there's been some issue with acquisition job trying to access a indicator dir that's being written to by an indicator run at the same time on Cronicle, causing failure to both processes. This change allows the option to limit such checks to one specific indicator_dir under data_dir/receiving only.
This fix hopefully will help separate these processes as much as possible so they don't step on each other's foot.
I have tested both the optional arg and lacking thereof. test_csv_uploading.py pass in both cases, meaning if the argument is there, csv_uploading works, and if the argument is not there, csv_uploading also works, so it's pretty safe to add this optional argument.
There was a problem hiding this comment.
whoops! that comment was supposed to be attached to line 33 -- i was asking about the nargs argument to the add_argument() call. 🤦
There was a problem hiding this comment.
@melange396 Oh that just means it's an optional argument, so if you dont include the argument, it would still run as it does currently.
There was a problem hiding this comment.
specifying default does that by itself, making nargs unnecessary. i think you should take it out to avoid confusion but it doesnt break anything to leave it there. ¯\_(ツ)_/¯
Co-authored-by: melange396 <[email protected]>
Co-authored-by: melange396 <[email protected]>
Co-authored-by: melange396 <[email protected]>
| help="filename for log output (defaults to stdout)") | ||
| parser.add_argument( | ||
| '--indicator_name', | ||
| nargs='?', |
There was a problem hiding this comment.
specifying default does that by itself, making nargs unnecessary. i think you should take it out to avoid confusion but it doesnt break anything to leave it there. ¯\_(ツ)_/¯
3 participants
This change helps acquisition switch to running per-indicator on Cronicle.
Add optional argument indicator_name to allow acquisition run on one specified indicator only. If this argument is not there, acquisition behavior does not change from current implementation, i.e. acquisition looks through all indicators inside data_dir for new csv files.
Additionally, to test the above addition accurately, this PR also replaces this magicmock test object with an argparse object in test_csv_uploading.