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

[SYNPY-1358] Correction of timestamp in annotations from manifest file #1020

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

BryanFauble
Copy link
Contributor

@BryanFauble BryanFauble commented Dec 5, 2023

Problem:

  1. When a timestamp is given in a manifest.tsv file the current code is storing the data as a string annotation instead of a date.
  2. When a datetime or date object is created we did not account for the timezone of the current machine or what is defined in the datetime object.
  3. When the UTC timestamp was coming back from the synapse API the code was always converting the timestamp into local time, however, UTC time is what synapse is giving us.

Solution:

  1. I added logic when creating timestamps from the manifest.tsv file to check the format of the string. If it matches a date format we are going to use that type in order to create the annotation.
  2. I am taking into account the timezone of the current machine or, if defined, the timezone defined on the datetime object.
  3. For data coming back from the synapse server we are now taking it in as UTC time which allows anyone who needs to use it have the correct time. This also allows for the timestamp to correct print in the manifest.tsv

Testing:

  1. I manually tested with several annotations. For example given this manifest file:
path    parent  name    id      synapseStore    contentType     used    executed        activityName    activityDescription     my_key_timestamp_date_yesterday my_key_long     my_key_bool     my_key_string   my_key_double   my_key_timestamp_datetime       my_key_timestamp_date
/home/bfauble/annotationTesting/file.txt        syn52919599     file.txt        syn53085003     True    text/plain                                      2023-12-04T07:00:00Z    1       False   b       1.2     2023-12-05 23:37:02.995000+00:00        2023-12-05 07:00:00+00:00

image
2. I update the unit and integration tests to allow for this logic.

@BryanFauble BryanFauble requested a review from a team as a code owner December 5, 2023 22:39
@pep8speaks
Copy link

pep8speaks commented Dec 5, 2023

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1062:89: E501 line too long (141 > 88 characters)
Line 1063:89: E501 line too long (121 > 88 characters)
Line 1064:89: E501 line too long (141 > 88 characters)
Line 1065:89: E501 line too long (141 > 88 characters)
Line 1066:89: E501 line too long (141 > 88 characters)
Line 1067:89: E501 line too long (141 > 88 characters)
Line 1068:89: E501 line too long (141 > 88 characters)

Line 4:10: E401 multiple imports on one line

Line 37:89: E501 line too long (125 > 88 characters)
Line 39:89: E501 line too long (162 > 88 characters)
Line 166:89: E501 line too long (94 > 88 characters)

Line 7:10: E401 multiple imports on one line

Comment last updated at 2023-12-06 16:07:21 UTC

synapseclient/core/utils.py Show resolved Hide resolved
@BryanFauble BryanFauble merged commit 219fd89 into develop Dec 6, 2023
35 checks passed
@BryanFauble BryanFauble deleted the SYNPY-1358-annotation-timestamp branch December 6, 2023 20:49
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.

3 participants