LBRYCommunity/ytsync
1
0
Fork 0
Code Issues 39 Pull requests 8 Projects Releases 197 Packages Wiki Activity

Download video to sync to local cache #113

Open
pseudoscalar wants to merge 7 commits from pseudoscalar/issue/112/v1 into local
pull from: pseudoscalar/issue/112/v1
merge into: LBRYCommunity:local
Conversation 1 Commits 7 Files changed 7 +820 -5
pseudoscalar commented 2021-10-14 20:07:35 +02:00 (Migrated from github.com)
Copy link

Partial completion of v1 of issue #112. Drafting this PR for discussion.

Partial completion of v1 of issue #112. Drafting this PR for discussion.
lyoshenka (Migrated from github.com) reviewed 2021-10-14 21:42:24 +02:00
local/local.go
lyoshenka (Migrated from github.com) commented 2021-10-14 21:42:24 +02:00
Copy link

i recommend os.ReadFile() to read the file in one go

i recommend os.ReadFile() to read the file in one go
pseudoscalar commented 2021-10-25 23:30:51 +02:00 (Migrated from github.com)
Copy link

Posting some work before I get too far off into the weeds. There are some comments about improvements that could be made to the go jsonrpc client, and stuff I still need to do for the upload. This is due for a refactor, I just wanted to get the whole process out into code before figuring out how to organize it.

Posting some work before I get too far off into the weeds. There are some comments about improvements that could be made to the go jsonrpc client, and stuff I still need to do for the upload. This is due for a refactor, I just wanted to get the whole process out into code before figuring out how to organize it.
lyoshenka (Migrated from github.com) reviewed 2021-11-05 21:42:52 +01:00
local/local.go
lyoshenka (Migrated from github.com) commented 2021-11-05 21:32:31 +01:00
Copy link

id call this something other than done to make it clear its a channel that lets you know when the video is done reflecting

id call this something other than `done` to make it clear its a channel that lets you know when the video is done reflecting
lyoshenka (Migrated from github.com) commented 2021-11-05 21:37:50 +01:00
Copy link

minor nit, but doesn't this potentially make the description longer than the max length?

minor nit, but doesn't this potentially make the description longer than the max length?
local/local.go
@ -22,0 +86,4 @@
videoID := args[0]
log.Debugf("Running sync for video ID %s", videoID)
lyoshenka (Migrated from github.com) commented 2021-11-05 21:31:22 +01:00
Copy link

prolly need to make sure this exists

prolly need to make sure this exists
local/localSDKPublisher.go
lyoshenka (Migrated from github.com) commented 2021-11-05 21:35:00 +01:00
Copy link

if this is an error (or an unexpected thing), it should prolly send an error to done, right?

if this is an error (or an unexpected thing), it should prolly send an error to `done`, right?
local/youtubeEnricher.go
@ -0,0 +18,4 @@
func NewYouTubeAPIVideoEnricher(apiKey string) (*YouTubeAPIVideoEnricher) {
enricher := YouTubeAPIVideoEnricher{
api: NewYouTubeAPI(apiKey),
lyoshenka (Migrated from github.com) commented 2021-11-05 21:39:51 +01:00
Copy link

so the best way to get the release time was via the api? if so, can you include instructions in your PR on what a user would need to do to get an api key? we'd also want to add handling in a later version for exceeding api limits

so the best way to get the release time was via the api? if so, can you include instructions in your PR on what a user would need to do to get an api key? we'd also want to add handling in a later version for exceeding api limits
local/ytdlVideoSource.go
@ -0,0 +59,4 @@
FullLocalPath: videoPath,
}
for _, enricher := range s.enrichers {
lyoshenka (Migrated from github.com) commented 2021-11-05 21:42:48 +01:00
Copy link

this is an interesting design. what other enrichers did you envision?

this is an interesting design. what other enrichers did you envision?
pseudoscalar (Migrated from github.com) reviewed 2021-11-08 16:37:40 +01:00
local/local.go
@ -22,0 +86,4 @@
videoID := args[0]
log.Debugf("Running sync for video ID %s", videoID)
pseudoscalar (Migrated from github.com) commented 2021-11-08 16:37:40 +01:00
Copy link

This should be guaranteed by cobra.ExactArgs(1), but I can a check and error here too.

This should be guaranteed by `cobra.ExactArgs(1)`, but I can a check and error here too.
pseudoscalar (Migrated from github.com) reviewed 2021-11-08 16:44:35 +01:00
local/local.go
pseudoscalar (Migrated from github.com) commented 2021-11-08 16:44:35 +01:00
Copy link

This was mostly ripped from getAbbrevDescription in sources/youtubeVideo.go. I didn't know exactly where the value of 2800 came from, so I just assumed the logic was correct. If it is supposed to be a limit on the total description length, I'll fix this.

This was mostly ripped from `getAbbrevDescription` in sources/youtubeVideo.go. I didn't know exactly where the value of 2800 came from, so I just assumed the logic was correct. If it is supposed to be a limit on the total description length, I'll fix this.
pseudoscalar (Migrated from github.com) reviewed 2021-11-08 16:47:59 +01:00
local/youtubeEnricher.go
@ -0,0 +18,4 @@
func NewYouTubeAPIVideoEnricher(apiKey string) (*YouTubeAPIVideoEnricher) {
enricher := YouTubeAPIVideoEnricher{
api: NewYouTubeAPI(apiKey),
pseudoscalar (Migrated from github.com) commented 2021-11-08 16:47:59 +01:00
Copy link

This was the simplest and most direct way, and the API limits seem like more than enough for the single channel use-case (except on initial channel sync). Should I add the instructions to the local/readme.md?

This was the simplest and most direct way, and the API limits seem like more than enough for the single channel use-case (except on initial channel sync). Should I add the instructions to the local/readme.md?
pseudoscalar (Migrated from github.com) reviewed 2021-11-08 17:04:52 +01:00
local/ytdlVideoSource.go
@ -0,0 +59,4 @@
FullLocalPath: videoPath,
}
for _, enricher := range s.enrichers {
pseudoscalar (Migrated from github.com) commented 2021-11-08 17:04:52 +01:00
Copy link

This mainly came out of trying to accommodate easy switching between methods of getting release date and also allowing for fallback to multiple methods. The other methods in mind were things like the odysee API, multiple YouTube API keys, a future update to yt-dlp that might provide it, web scraping (although I don't think this would currently work as only the date is provided), or some 3rd party metadata provider. In the single video use-case, there could even be an enricher that just supplies values specified on the command line.

The design here is meant to put together a list of enrichers based on command line options. The enrichers will then take turns trying fill-in the missing video metadata (although right now, it is only trying to fill-in release date)

This mainly came out of trying to accommodate easy switching between methods of getting release date and also allowing for fallback to multiple methods. The other methods in mind were things like the odysee API, multiple YouTube API keys, a future update to yt-dlp that might provide it, web scraping (although I don't think this would currently work as only the date is provided), or some 3rd party metadata provider. In the single video use-case, there could even be an enricher that just supplies values specified on the command line. The design here is meant to put together a list of enrichers based on command line options. The enrichers will then take turns trying fill-in the missing video metadata (although right now, it is only trying to fill-in release date)
pseudoscalar (Migrated from github.com) reviewed 2021-11-08 17:17:49 +01:00
local/localSDKPublisher.go
pseudoscalar (Migrated from github.com) commented 2021-11-08 17:17:49 +01:00
Copy link

The thinking was that this may be intentional since it is driven by lbrynet settings. I could turn this into an error if that case is unlikely. I could also add a command line flag to indicate that streams won't be reflected.

The thinking was that this may be intentional since it is driven by lbrynet settings. I could turn this into an error if that case is unlikely. I could also add a command line flag to indicate that streams won't be reflected.
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:09:58 +01:00
local/local.go
@ -22,0 +86,4 @@
videoID := args[0]
log.Debugf("Running sync for video ID %s", videoID)
lyoshenka (Migrated from github.com) commented 2021-11-11 23:09:58 +01:00
Copy link

you're right, i just didnt notice that

you're right, i just didnt notice that
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:11:09 +01:00
local/localSDKPublisher.go
lyoshenka (Migrated from github.com) commented 2021-11-11 23:11:09 +01:00
Copy link

yea id either make it an option or just assume that all streams will be reflected. option is better but more work

yea id either make it an option or just assume that all streams will be reflected. option is better but more work
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:12:04 +01:00
local/local.go
lyoshenka (Migrated from github.com) commented 2021-11-11 23:12:04 +01:00
Copy link

i think that's an estimate of how long the description can get before we hit the max claim size limit. id still fix it so the math is right though :-)

i think that's an estimate of how long the description can get before we hit the max claim size limit. id still fix it so the math is right though :-)
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:12:26 +01:00
local/youtubeEnricher.go
@ -0,0 +18,4 @@
func NewYouTubeAPIVideoEnricher(apiKey string) (*YouTubeAPIVideoEnricher) {
enricher := YouTubeAPIVideoEnricher{
api: NewYouTubeAPI(apiKey),
lyoshenka (Migrated from github.com) commented 2021-11-11 23:12:26 +01:00
Copy link

yea

yea
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:13:54 +01:00
local/youtubeEnricher.go
@ -0,0 +18,4 @@
func NewYouTubeAPIVideoEnricher(apiKey string) (*YouTubeAPIVideoEnricher) {
enricher := YouTubeAPIVideoEnricher{
api: NewYouTubeAPI(apiKey),
lyoshenka (Migrated from github.com) commented 2021-11-11 23:13:54 +01:00
Copy link

maybe add a note to the issue to give the user a good error message if the api runs out of credits, and stop the sync. getting the release time right is fairly important so we don't want to guess or leave it empty. this could go under v4

maybe add a note to the issue to give the user a good error message if the api runs out of credits, and stop the sync. getting the release time right is fairly important so we don't want to guess or leave it empty. this could go under v4
lyoshenka (Migrated from github.com) reviewed 2021-11-11 23:14:24 +01:00
local/ytdlVideoSource.go
@ -0,0 +59,4 @@
FullLocalPath: videoPath,
}
for _, enricher := range s.enrichers {
lyoshenka (Migrated from github.com) commented 2021-11-11 23:14:24 +01:00
Copy link

👍

:+1:
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin pseudoscalar/issue/112/v1:pseudoscalar/issue/112/v1
git checkout pseudoscalar/issue/112/v1

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout local
git merge --no-ff pseudoscalar/issue/112/v1
git checkout pseudoscalar/issue/112/v1
git rebase local
git checkout local
git merge --ff-only pseudoscalar/issue/112/v1
git checkout pseudoscalar/issue/112/v1
git rebase local
git checkout local
git merge --no-ff pseudoscalar/issue/112/v1
git checkout local
git merge --squash pseudoscalar/issue/112/v1
git checkout local
git merge --ff-only pseudoscalar/issue/112/v1
git checkout local
git merge pseudoscalar/issue/112/v1
git push origin local
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
dependencies

Pull requests that update a dependency file

  
Epic

   
good first issue

Good for newcomers

  
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

Extra attention is needed

  
icebox

Long-term storage

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
sdk dependent

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
sdk dependent
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/ytsync#113
No description provided.
Delete branch "pseudoscalar/issue/112/v1"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?