-
Notifications
You must be signed in to change notification settings - Fork 26
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
feature/process-local-file #89
feature/process-local-file #89
Conversation
Codecov Report
@@ Coverage Diff @@
## main #89 +/- ##
==========================================
- Coverage 95.88% 95.72% -0.17%
==========================================
Files 48 48
Lines 2238 2247 +9
==========================================
+ Hits 2146 2151 +5
- Misses 92 96 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- it's pretty neat that for this many lines of code we are adding a new feature, like go team on making it robust like this.
- one comment on general design of how to accept this file and pass to pipeline
- one nitpick comment on tempdir management, can ignore if you want.
Other than that, looks pretty sane to me.
OH! And when attaching the URI back to the session model, add the |
Unfortunately this does break the pipeline when we try to do |
Oh whoops. I event added it in the wrong place myself. It is needed for google file storage to recognize that you aren't trying to download or copy the file but rather stream the file. But what I meant by "I added it in the wrong place" is that you can keep it how it is for all the file storage and processing and such (exactly how it is now) but then during the database upload task, can you pass a parameter that says like "from_local" or something, and if it is local the session_video_uris need to be converted to their In [1]: from gcsfs import GCSFileSystem
In [2]: fs = GCSFileSystem(token="../cdp-backend/.keys/cdp-jackson-dev-002-sa-dev.json")
In [3]: fs.url("cdp-jackson-dev-002.appspot.com/9af4adb4f28b34c0142a2ab28fa5603dd5fdb347c074d957d13636dae554ed34-audio.wav")
Out[3]: 'https://storage.googleapis.com/download/storage/v1/b/cdp-jackson-dev-002.appspot.com/o/9af4adb4f28b34c0142a2ab28fa5603dd5fdb347c074d957d13636dae554ed34-audio.wav?alt=media' It even adds that TL;DR: it's need for the front-end streaming of the file and not the backend processing. So only change it right before database storage of the URI. |
Ah gotcha that makes sense, I didn't know it had that use case for the front end. Will do |
Just curious but does the front end have a way of streaming the video of something stored in GCS using the HTTP url? |
Not sure why the |
From: https://councildataproject.org/seattle/#/events/2ee8dfd2-a4a5-416f-aded-f48b37e6e521 |
They may have released something and something somewhere is broken in the dep chain, I will wait a bit and retrigger GitHub Actions to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to keep this open a second and test the ubuntu graphviz stuff tomorrow morning. But all looks good.
This actually looks like a github actions release failure.... GitHub released a new build of their ubuntu runner 6 days ago which aligns with when the ubuntu tests started failing.... https://github.com/actions/virtual-environments/releases/tag/ubuntu20%2F20210816.1 |
It's actually not |
Got it figured out. Going to merge this then merge my PR to fix the builds without merge conflicts. |
Link to Relevant Issue
This pull request resolves #57
Description of Changes
Adds a script for processing a local video file.
event_details_file
is just anEventIngestionModel
in JSON format.Example run:
I also made the
resource_copy
function create the file in a temporary directory ifdst
is not provided. I did this because otherwise theresource_copy
will copy into the repo directory. This usually isn't a problem, but for example if the video file provided in the script lives incdp_backend
, then we'll get aFileExistsError
during resource copy and the pipeline will error out.