CouncilDataProject / cdp-backend

Data storage utilities and processing pipelines used by CDP instances.
https://councildataproject.org/cdp-backend
Mozilla Public License 2.0
22 stars 26 forks source link

bugfix/unique-temp-filenames #225

Closed chrisjkhan closed 1 year ago

chrisjkhan commented 1 year ago

This pull request resolves #226

Use content hash for temp video file naming to prevent collisions across

codecov[bot] commented 1 year ago

Codecov Report

Merging #225 (4cdb0ab) into main (b00f378) will increase coverage by 0.28%. The diff coverage is 95.91%.

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   72.40%   72.68%   +0.28%     
==========================================
  Files          64       64              
  Lines        3541     3581      +40     
==========================================
+ Hits         2564     2603      +39     
- Misses        977      978       +1     
Impacted Files Coverage Δ
...ckend/tests/pipeline/test_event_gather_pipeline.py 99.17% <88.88%> (-0.83%) :arrow_down:
cdp_backend/utils/file_utils.py 87.94% <92.30%> (+0.68%) :arrow_up:
cdp_backend/pipeline/event_gather_pipeline.py 84.74% <100.00%> (+0.03%) :arrow_up:
cdp_backend/tests/utils/test_file_utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

chrisjkhan commented 1 year ago

Looks good to me! @dphoria can you also check this. Will merge soon!

@evamaxfield actually, it's not working right now. I was wanting to reach out to see if you knew what might be wrong. The temporary video file exists in the context of the convert_video_and_handle_host function, after the resource_copy_task, but it does not exist in the context of the file_utils.clip_and_reformat_video function, with no other commands called in between. I'm starting to think it's something small, like a typo. I thought maybe it had to do with the Prefect architecture, but I don't think so now. It is essentially the same architectural form as before any changes were made for trimming, so it seems like it must be a minor issue.

File exists here: https://github.com/CouncilDataProject/cdp-backend/blob/01595c3d6653649c4fab376ac686cfd5ada91f9a/cdp_backend/pipeline/event_gather_pipeline.py#L358 But not here: https://github.com/CouncilDataProject/cdp-backend/blob/01595c3d6653649c4fab376ac686cfd5ada91f9a/cdp_backend/utils/file_utils.py#L724

@dphoria let me know if you have any ideas

dphoria commented 1 year ago

:rofl: I have failed as a reviewer. Anyway, good catch Chris. I'm preoccupied with some other stuff today; will try to debug when I can later today. Hopefully you'll figure it out soon. :sweat_smile:

chrisjkhan commented 1 year ago

Relevant logs: https://github.com/OpenMontana/montana-legislature-council-data-project/actions/runs/3811636069/jobs/6484420314#step:9:81

evamaxfield commented 1 year ago

I will also try to take a look asap

chrisjkhan commented 1 year ago

Looking good: https://github.com/OpenMontana/montana-legislature-council-data-project/actions/runs/3814172947/jobs/6488287009

evamaxfield commented 1 year ago

Thanks for making all the changes @chrisjkhan!

Merging and releasing new version. Upgrade should rollout in ~2 hours.