[BD-27] Upload transcripts downloaded by the video_download script #61

janimo · 2021-04-19T07:02:53Z

This will look for .srt files downloaded by youtube-dl along with the videos and will upload them after each video is uploaded.
I have only tested this with a devstack, checking that the transcript_uploads/ endpoint is called successfully.

I've also added tests for argument parsing to increase CI coverage and have the checks pass.

openedx-webhooks · 2021-04-19T07:02:59Z

Thanks for the pull request, @janimo! I've created BLENDED-828 to keep track of it in Jira. More details are on the BD-27 project page.

When this pull request is ready, tag your edX technical lead.

alangsto · 2021-05-21T13:08:19Z

@janimo this looks good - but a few questions.

Before merging it would be great if this could be tested on stage/edge
Could we change the file names of the text fixtures? I think they too closely resemble actual video names from a course.

janimo · 2021-05-21T13:25:52Z

@alangsto it would be great if I could get test on either stage or edge but I'm afraid I do not have an account there. Would I need to go through the process described here to get API credentials?

https://course-catalog-api-guide.readthedocs.io/en/latest/authentication/#getting-a-client-id-and-client-secret

Do you mean the names of the .srt files added under tests/fixtures_data/video_files/01___Intro_to_Knowledge_Based_AI/ ? If so I have used names that mirror the video file name but with a different exceptions, since this is what I saw how youtube-dl saves transcripts for videos and it makes associating them relatively straightforward.

alangsto · 2021-05-21T13:33:00Z

@janimo

Yes, you would need to get the API credentials to test there. My understanding was that this was being tested this week, potentially by @kaizoku, is that correct?

And I see, then the .srt filenames are fine.

janimo · 2021-05-21T13:48:13Z

@alangsto ok, @kaizoku has an account, so probably we can test using API credentials associated with that.

kaizoku · 2021-05-22T12:49:39Z

@alangsto , I was attempting to test on my devstack first, but underestimated the complexity of setting up the video pipeline.

Though, I get a 404 on edge, and staging for the API admin: https://edge.edx.org/api-admin/
Is there somewhere else to generate the API credentials for edge and staging?

alangsto · 2021-05-24T12:47:17Z

@kaizoku, no worries about testing on edge or stage then, we can take that on as part of our testing of this tool.

@janimo one last request, could you please update the documentation for the video + transcript tooling here in this file (https://github.com/edx/cc2olx/blob/master/src/cc2olx/tools/README.rst). Thanks!

janimo · 2021-05-24T13:16:42Z

@alangsto I've updated the README to mention transcripts. One thing I am not sure about is what other (if any) changes are needed in the cc2olx tool to fully support transcripts. The documentation I used at https://edx.readthedocs.io/projects/edx-open-learning-xml/en/latest/components/video-components.html doesn't explain much about transcripts in video blocks, and I did not find online examples of video blocks with transcript support enabled. While uploading a transcript will associate the video with it via de edx id tag, I am still not sure whether the video xblock should explicitly list the transcripts itself.

alangsto · 2021-05-24T18:30:02Z

@janimo have you tried adding a fake transcript to a video component in studio, and looking at the resulting olx from the course export? That would be my first suggestion to understand how xblocks and transcripts work together. I myself am not familiar, but had thought this would be included in the work for this ticket.

kaizoku · 2021-05-25T18:58:55Z

@janimo, yes, video blocks do need to specify the transcripts associated to the video.
I exported the example video block created when adding a new video block to a course and this is the resulting OLX:

<video url_name="a8f5779265854276b67407e6c24d544f" sub="" transcripts="{&quot;en&quot;: &quot;ac0e4345-102e-4990-9b80-97acf49bf678-en.srt&quot;}" display_name="Video" edx_video_id="ac0e4345-102e-4990-9b80-97acf49bf678" html5_sources="[]" youtube_id_1_0="3_yD_cEKoCk">
  <video_asset client_video_id="external video" duration="0.0" image="">
    <transcripts>
      <transcript language_code="en" file_format="srt" provider="Custom"/>
    </transcripts>
  </video_asset>
  <transcript language="en" src="ac0e4345-102e-4990-9b80-97acf49bf678-en.srt"/>
</video>

We'll need to add OLX generation to link the transcripts to the associated video here.

This add the new column that contains dash separated, alphabetically ordered language codes to the output CSV. This is used by cc2olx to add transcript tags in the generated video block.

janimo · 2021-05-27T09:36:39Z

I've added a related pull which addresses the cc2olx part, once the video upload script generates a CSV with language codes.
#66

kaizoku · 2021-05-31T15:43:25Z

@janimo, I setup the video pipeline in my devstack and it looks like the transcript upload endpoint doesn't accept the API credentials, as it uses the basic django auth_required, rather than the API auth classes used by the video upload handler.

It looks like we'll need to find another way to upload transcripts programmatically. Did you find any other endpoints while working on this?

janimo · 2021-06-01T05:41:37Z

@kaizoku I did not find another endpoint, but I did not look either after I found this one TBH.

alangsto · 2021-06-01T13:12:12Z

@kaizoku @janimo do you need further help on the development of this? I was hoping to test the tool this week, but can set aside time to assist you both if needed. Please let me know

alangsto · 2021-06-07T18:50:13Z

src/cc2olx/tools/README.rst

 -----------------

-The video upload tool uploads video files to edX's video encoding pipeline.
+The video upload tool uploads video files and associated transcripts to edX's video encoding pipeline.


@kaizoku can we also include documentation for the video download tool? It can be added to this file, but maybe under another section

Yes, I added a section below describing the options to the video download tool, its expected input and output, and how it can be used with the video upload tool. Does this extra section look good?

That looks great, thanks for adding!

alangsto · 2021-06-07T18:51:19Z

src/cc2olx/tools/video_upload.py

+    Arguments:
+        * filename: the transcript filename
+        * edx_video_id: the video ID of the video this transcript is for
+        * language_code: the language of the transcript


Please add access token to docstring args

Thanks for catching that. It's added now.

alangsto · 2021-06-07T18:52:31Z

tests/test_tools.py

+    mocker.patch("cc2olx.tools.video_upload.open")
+    mocker.patch("cc2olx.tools.video_upload.requests.post", side_effect=upload_transcript_side_effect)
+
+    response = upload_transcript("filename", "edxid", "en")


Missing access_token arg

alangsto · 2021-06-07T18:53:31Z

src/cc2olx/tools/README.rst

 The ``course-id`` argument is the ID of the course as it appears in Studio. For example, ``course-v1:edX+111222+111222``.

-The ``directory`` argument is a directory containing video files that will be uploaded to edX's video encoding pipeline.
+The ``directory`` argument is a directory (such as the one created by the video download tool) containing video files and transcripts, that will be uploaded to edX's video encoding pipeline.


How should this directory be structured? Will all of the video files and transcripts be within the same directory?

I added some clarification to it's file searching behavior in this section. Does that help on this point?

alangsto · 2021-06-10T12:26:46Z

@kaizoku this looks good, I can merge once you fix the formatting

kaizoku · 2021-06-12T02:44:13Z

Ah sorry for the formatting @alangsto. I just noticed make test doesn't run the format checks.

I've fixed the formatting issue here.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 19, 2021

janimo force-pushed the jani/video-upload-transcripts branch 5 times, most recently from 181bf45 to b9a68ea Compare May 17, 2021 12:56

janimo changed the title ~~[BD-27] WIP Upload downloaded transcripts~~ [BD-27] Upload transcripts downloaded by the video_download script May 17, 2021

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 17, 2021

Jani Monoses added 5 commits May 27, 2021 09:22

Upload video transcripts if found

2b7b03d

Add more tests for video upload tool.

225557d

Fix an old typo

47fec5d

Update readme to mention transcript uploads

45483c8

Add transcript languages to video upload output CSV

87f65ee

This add the new column that contains dash separated, alphabetically ordered language codes to the output CSV. This is used by cc2olx to add transcript tags in the generated video block.

janimo force-pushed the jani/video-upload-transcripts branch 2 times, most recently from 074b2a0 to 87f65ee Compare May 27, 2021 08:48

janimo mentioned this pull request May 27, 2021

[BD-27] Create transcript nodes in video blocks #66

Merged

kaizoku mentioned this pull request Jun 4, 2021

[BD-27] FEAT: Add new API endpoint for uploading transcripts openedx/openedx-platform#27844

Merged

kaizoku added 2 commits June 7, 2021 07:15

Use Authorization header for transcript uploads

a5a041e

Use newly created transcript upload endpoint

8489087

alangsto reviewed Jun 7, 2021

View reviewed changes

alangsto suggested changes Jun 7, 2021

View reviewed changes

alangsto reviewed Jun 7, 2021

View reviewed changes

kaizoku added 3 commits June 9, 2021 14:59

Update tests for new transcript_upload function

e770f1f

Update documentation for video download tool

7a20adf

Add clearer heading hierarchy in tools README

a3cdb57

Fix tests formatting

c663606

alangsto approved these changes Jun 14, 2021

View reviewed changes

alangsto merged commit 7567270 into openedx:master Jun 14, 2021

openedx-webhooks added merged and removed needs triage labels Jun 14, 2021

[BD-27] Upload transcripts downloaded by the video_download script #61

[BD-27] Upload transcripts downloaded by the video_download script #61

Uh oh!

Conversation

janimo commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alangsto commented May 21, 2021

Uh oh!

janimo commented May 21, 2021

Uh oh!

alangsto commented May 21, 2021

Uh oh!

janimo commented May 21, 2021

Uh oh!

kaizoku commented May 22, 2021

Uh oh!

alangsto commented May 24, 2021

Uh oh!

janimo commented May 24, 2021

Uh oh!

alangsto commented May 24, 2021

Uh oh!

kaizoku commented May 25, 2021

Uh oh!

janimo commented May 27, 2021

Uh oh!

kaizoku commented May 31, 2021

Uh oh!

janimo commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alangsto commented Jun 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alangsto commented Jun 10, 2021

Uh oh!

kaizoku commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janimo commented Apr 19, 2021 •

edited

Loading

openedx-webhooks commented Apr 19, 2021 •

edited

Loading

janimo commented Jun 1, 2021 •

edited

Loading