search

ANNIEsoft / ToolAnalysis

Other
10 stars 53 forks source link

Modification to the BeamFetcher Tool, adjusting for the SQL database now being in UTC #233

Closed S81D closed 1 year ago

S81D commented 1 year ago

The BeamFetcher toolchain fetches the starting time and end times for a run according to a .txt file (ANNIE_RunInformation_PSQL.txt) which is traditionally copied from the SQL database. Since the database is now in UTC, there no longer is a need to convert the SQL times (previously Chicago time). Also, there is a configuration variable in the BeamFetcher Config file that applies a time shift if there is Daylight Savings, so that is also no longer needed.

marc1uk commented 1 year ago

sorry to ping with such a minor change. I'd normally just make it myself since you've given me permission to edit your branch directly, but since it's your main Application branch i don't want to cause you problems in case you're making other modifications on it. Can we remove this comment, i don't see the need to reference features that no longer exist in the codebase. Similarly leaving a commented out variable that isn't needed is just code cruft; please just delete it.

So my understanding is these changes rely on the file read by the LoadRunInfo Tool to be in UTC, which relies on the correct SQL query being used when querying run start/stop timestamps from the database. I can't see anything in the README for that tool which specifies a suitable query. The README actually suggests copy/paste-ing from the webpage, but from a quick check it seems like the webpage does not show timestamps in UTC.

S81D commented 1 year ago

I can go ahead and remove that comment + commented out variable. It was my understanding that the SQL now displays timestamps in UTC based on the discussions in the software channel (per Michael and your exchange in late April), but it appears that I was wrong and it looks to be in Europe/London time.

I believe the LoadRunInfo Tool doesn't have to be in UTC when that information is passed to BeamFetcher - the conversion is done internally from (what was) Chicago time to UTC. I think given the SQL times are really London/Europe this PR should probably be dismissed then. I can re-adjust BeamFetcher to convert from Europe/London to UTC instead of Chicago to UTC.

S81D commented 1 year ago

Sent this just as you sent the message in Software. I'll go ahead and make the changes.

marc1uk commented 1 year ago

i've updated the webpage, but really, we should not be hard-coding timezone changes anywhere.

the SQL times are really London/Europe

Following the updates you mentioned discussed in slack timestamps in the database are stored internally as UTC - it's just that the webpage doesn't request them in UTC. Ultimately, it doesn't really matter what the database internally stores things as - provided it is queried in the right way, it can return timestamps in UTC. All our code should work in UTC, and the input to those Tools should be in UTC. All timestamps in UTC should be the standard, so please don't revert anything to PST/BST; if there are such conversions, please remove them.