Convert datetime values to strings before indexing (#3416) #3419

dsotirho-ucsc · 2021-09-10T17:04:16Z

#3416

PR title references issue
PR title matches issue title (preceded by Fix: for bugs) _{or there is a good reason why they're different}
Title of main commit references issue
PR is connected to Zenhub issue and description links to issue

Author (reindex)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}

Author (freebies & chains)

Freebies are blocked on this PR _{or there are no freebies in this PR}
Freebies are referenced in commit titles _{or there are no freebies in this PR}
This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added chain label to the blocking PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}
Added announcement to PR description _{or this PR does not require announcement}

Author (requirements, before every review)

Ran make requirements_update _{or this PR leaves requirements*.txt, common.mk and Makefile untouched}
Added R tag to commit title _{or this PR leaves requirements*.txt untouched}
Added reqs label to PR _{or this PR leaves requirements*.txt untouched}

Author (before every review)

make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}
Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

Commented in issue about demo expectations _{or labelled issue as no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
Moved ticket to Approved column
Assigned PR to an operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Made announcement requested by author _{or PR description does not contain an announcement}
Moved freebies to Merged column _{or there are no freebies in this PR}
Shortened the PR chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate
Pushed merge commit to Gitlab _{or this changes can be pushed later, together with another PR}
Deleted PR branch from Github and Gitlab

Operator (reindex)

Started reindex in dev _{or this PR does not require reindexing or does not target dev}
Checked for failures in dev _{or this PR does not require reindexing or does not target dev}
Started reindex in prod _{or this PR does not require reindexing or does not target prod}
Checked for failures in prod _{or this PR does not require reindexing or does not target prod}

Operator

Unassigned PR

codecov · 2021-09-10T17:23:27Z

Codecov Report

Merging #3419 (e7a3355) into develop (0a40364) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e7a3355 differs from pull request most recent head 3f17533. Consider uploading reports for the commit 3f17533 to get more accurate results

@@             Coverage Diff             @@
##           develop    #3419      +/-   ##
===========================================
- Coverage    82.66%   82.65%   -0.01%     
===========================================
  Files          123      123              
  Lines        14218    14212       -6     
===========================================
- Hits         11753    11747       -6     
  Misses        2465     2465

Impacted Files	Coverage Δ
src/azul/service/hca_response_v5.py	`92.41% <ø> (-0.11%)`	⬇️
src/azul/time.py	`76.47% <ø> (ø)`
src/azul/indexer/document.py	`95.89% <100.00%> (+0.01%)`	⬆️
src/azul/plugins/metadata/hca/transform.py	`98.64% <100.00%> (+0.02%)`	⬆️
src/azul/service/avro_pfb.py	`95.76% <100.00%> (-0.43%)`	⬇️
test/service/test_manifest_async.py	`100.00% <0.00%> (ø)`
src/azul/service/manifest_service.py	`94.75% <0.00%> (ø)`

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 0a40364...3f17533. Read the comment docs.

coveralls · 2021-09-10T17:23:52Z

Coverage remained the same at 82.885% when pulling 3f17533 on issues/danielsotirhos/3416-convert-datetime-to-string into 0a40364 on develop.

achave11-ucsc

Looks good. Only made an optional suggestion.

achave11-ucsc · 2021-09-14T23:54:00Z

src/azul/indexer/document.py

        if value is None:
            return self.null
        else:
-            return format_dcp2_datetime(value)
+            parse_dcp2_datetime(value)  # to verify a valid dcp2 datetime string


I am not sure that this comment is necessary. Up to you.

hannes-ucsc · 2021-09-17T05:04:00Z

src/azul/time.py

+    if d is None:
+        return None
+    else:


A fixpoint like this is a smell. If None is a fixpoint in one function why is it not a fixpoint of every function?

Also, format and parse functions should be each other's inverse and this breaks that.

hannes-ucsc · 2021-09-17T23:25:06Z

src/azul/plugins/metadata/hca/transform.py

+            update_date = format_dcp2_datetime(entity.update_date)
+        return {
+            'submission_date': format_dcp2_datetime(entity.submission_date),
+            'update_date': update_date,


Suggested change

'update_date': update_date,

'update_date': None

if entity.update_date is None else

format_dcp2_datetime(entity.update_date),

hannes-ucsc · 2021-09-17T23:25:58Z

src/azul/time.py

+    >>> format_dcp2_datetime(None)
+    Traceback (most recent call last):
+    ...
+    AttributeError: 'NoneType' object has no attribute 'tzinfo'
+


Suggested change

>>> format_dcp2_datetime(None)

Traceback (most recent call last):

...

AttributeError: 'NoneType' object has no attribute 'tzinfo'

jessebrennan · 2021-09-23T16:41:08Z

Failures during reindexing in dev:

[
  {
    "spec": "tdr:broad-jade-dev-data:snapshot/hca_dev_20201203___20210524_lattice:/2",
    "uuid": "40733888-3b9f-500e-84fd-1a18a64aadbe"
  },
  {
    "spec": "tdr:broad-jade-dev-data:snapshot/hca_dev_20201203___20210524_lattice:/2",
    "uuid": "94565940-7b2f-5fd7-beb1-dc309687fe09"
  },
  {
    "spec": "tdr:broad-jade-dev-data:snapshot/hca_dev_20201203___20210524_lattice:/2",
    "uuid": "9d0f5cd1-0ef6-5eed-8dd7-f15d36a34185"
  },
  {
    "spec": "tdr:broad-jade-dev-data:snapshot/hca_dev_20201203___20210524_lattice:/2",
    "uuid": "ae338c4e-650f-545b-8631-3c8d755aa2f9"
  },
  {
    "spec": "tdr:broad-jade-dev-data:snapshot/hca_dev_20201203___20210524_lattice:/2",
    "uuid": "c12a6ca2-3234-5182-ad0b-f8c2fb881ab5"
  }
]

These failure are expected because of:

hannes-ucsc · 2021-09-23T16:51:00Z

What's uuid above?

jessebrennan · 2021-09-23T18:36:16Z

It's the bundle UUID. To produce this I ran:

jq '[.messages[].Body.notification|{"spec": .source.spec, "uuid": .match.bundle_uuid}]' azul-notifications_fail-dev.json

github-actions bot added the orange [process] Done by the Azul team label Sep 10, 2021

dsotirho-ucsc added reindex:dev [process] PR requires reindexing dev base [process] Another PR needs to be rebased before merging this one labels Sep 10, 2021

dsotirho-ucsc requested a review from amarjandu September 10, 2021 19:37

dsotirho-ucsc assigned amarjandu Sep 10, 2021

dsotirho-ucsc mentioned this pull request Sep 10, 2021

Add content_description for matrices to /project endpoint (#3303) #3380

Merged

48 tasks

dsotirho-ucsc removed the request for review from amarjandu September 10, 2021 22:21

dsotirho-ucsc unassigned amarjandu Sep 10, 2021

dsotirho-ucsc removed the base [process] Another PR needs to be rebased before merging this one label Sep 14, 2021

dsotirho-ucsc force-pushed the issues/danielsotirhos/3416-convert-datetime-to-string branch from c808295 to 521df86 Compare September 14, 2021 22:19

dsotirho-ucsc requested a review from achave11-ucsc September 14, 2021 23:14

dsotirho-ucsc assigned achave11-ucsc Sep 14, 2021

achave11-ucsc approved these changes Sep 14, 2021

View reviewed changes

achave11-ucsc removed their assignment Sep 14, 2021

dsotirho-ucsc force-pushed the issues/danielsotirhos/3416-convert-datetime-to-string branch from 521df86 to ea3d269 Compare September 15, 2021 17:17

dsotirho-ucsc requested a review from hannes-ucsc September 15, 2021 18:09

dsotirho-ucsc assigned hannes-ucsc Sep 15, 2021

hannes-ucsc requested changes Sep 17, 2021

View reviewed changes

hannes-ucsc removed their assignment Sep 17, 2021

hannes-ucsc added 0 reviews [process] Lead didn't request any changes 1 review [process] Lead requested changes once and removed 0 reviews [process] Lead didn't request any changes labels Sep 17, 2021

dsotirho-ucsc requested a review from hannes-ucsc September 17, 2021 17:30

dsotirho-ucsc assigned hannes-ucsc Sep 17, 2021

hannes-ucsc requested changes Sep 17, 2021

View reviewed changes

hannes-ucsc added 2 reviews [process] Lead requested changes twice and removed 1 review [process] Lead requested changes once labels Sep 17, 2021

hannes-ucsc removed their assignment Sep 17, 2021

dsotirho-ucsc force-pushed the issues/danielsotirhos/3416-convert-datetime-to-string branch from e7a3355 to 73a24e4 Compare September 20, 2021 21:01

dsotirho-ucsc requested a review from hannes-ucsc September 21, 2021 16:37

dsotirho-ucsc assigned hannes-ucsc Sep 21, 2021

hannes-ucsc approved these changes Sep 21, 2021

View reviewed changes

hannes-ucsc assigned jessebrennan and unassigned hannes-ucsc Sep 21, 2021

dsotirho-ucsc added 2 commits September 21, 2021 16:40

[r] Convert datetime values to strings before indexing (#3416)

a249050

Fix typo in variable name

3f17533

jessebrennan force-pushed the issues/danielsotirhos/3416-convert-datetime-to-string branch from 73a24e4 to 3f17533 Compare September 21, 2021 23:40

jessebrennan added the sandbox [process] Resolution is being verified in sandbox deployment label Sep 21, 2021

jessebrennan merged commit bd666cd into develop Sep 22, 2021

jessebrennan deleted the issues/danielsotirhos/3416-convert-datetime-to-string branch September 22, 2021 20:07

jessebrennan removed their assignment Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert datetime values to strings before indexing (#3416) #3419

Convert datetime values to strings before indexing (#3416) #3419

dsotirho-ucsc commented Sep 10, 2021 •

edited by jessebrennan

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

coveralls commented Sep 10, 2021 •

edited

Loading

achave11-ucsc left a comment

achave11-ucsc Sep 14, 2021

hannes-ucsc Sep 17, 2021

hannes-ucsc Sep 17, 2021

hannes-ucsc Sep 17, 2021

jessebrennan commented Sep 23, 2021

hannes-ucsc commented Sep 23, 2021

jessebrennan commented Sep 23, 2021

-            'update_date': update_date,
+            'update_date': None
+                           if entity.update_date is None else
+                           format_dcp2_datetime(entity.update_date),

Convert datetime values to strings before indexing (#3416) #3419

Convert datetime values to strings before indexing (#3416) #3419

Conversation

dsotirho-ucsc commented Sep 10, 2021 • edited by jessebrennan Loading

codecov bot commented Sep 10, 2021 • edited Loading

Codecov Report

coveralls commented Sep 10, 2021 • edited Loading

achave11-ucsc left a comment

Choose a reason for hiding this comment

achave11-ucsc Sep 14, 2021

Choose a reason for hiding this comment

hannes-ucsc Sep 17, 2021

Choose a reason for hiding this comment

hannes-ucsc Sep 17, 2021

Choose a reason for hiding this comment

hannes-ucsc Sep 17, 2021

Choose a reason for hiding this comment

jessebrennan commented Sep 23, 2021

hannes-ucsc commented Sep 23, 2021

jessebrennan commented Sep 23, 2021

dsotirho-ucsc commented Sep 10, 2021 •

edited by jessebrennan

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

coveralls commented Sep 10, 2021 •

edited

Loading