Skip to content
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

Add Client.send_video high-level method #395

Merged
merged 6 commits into from
Sep 13, 2024
Merged

Conversation

Meorge
Copy link
Contributor

@Meorge Meorge commented Sep 12, 2024

closes #386. This PR adds a method, send_video, to the Client class that behaves very similarly to Client.send_image, allowing the user to easily upload a post with a video attached.

Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Please do not forget to provide async version too. Typically you just need to run client code gen. And please run ruff over new code

@MarshalX
Copy link
Owner

MarshalX commented Sep 12, 2024

@Meorge I fix it. async gen was a tricky one because of some hacks around upload_blob usage. Btw to run gen you can use poetry run atp gen async.

So the last question is do we want to do something with captions? Yes, the image has only alt, but video alt and captions. Do we want to add the argument for captions here?

I do not understand why "captions" is the array of files... do they expect some captions file format or just txt with timestamp - text...

@Meorge
Copy link
Contributor Author

Meorge commented Sep 12, 2024

@MarshalX Thanks for the fixes!

So the last question is do we want to do something with captions? Yes, the image has only alt, but video alt and captions. Do we want to add the argument for captions here?

As for the captions, it sounds to me like they'd want a list of caption files' blobs, probably in .srt format, with their associated language code attached. So a video embed object with captions might look like:

models.AppBskyEmbedVideo.Main(
  video=video_upload_blob,
  alt=video_alt,
  captions=[
    models.AppBskyEmbedVideo.Caption(file=english_captions_blobref, lang='en'),
    models.AppBskyEmbedVideo.Caption(file=spanish_captions_blobref, lang='es'),
    models.AppBskyEmbedVideo.Caption(file=french_captions_blobref, lang='fr'),
  ],
)

In this case, there would be captions available for the video in English, Spanish, and French.
(Note that this is just what I'd expect, given the structure of the API - I haven't tested this yet, so I can't be certain.)

As for whether to include them: I think they'd be great to add in a follow-up PR; it shouldn't be a breaking change to add support for them later on. But for now, providing some quick and easy way to post videos through the convenience method we have so far should be enough for most use cases. If the user has a use case where they do need to include captions, they can still use the lower-level methods to do that for now.

@MarshalX
Copy link
Owner

@Meorge don't you get this warning?

UserWarning: Pydantic serializer warnings:
  Failed to get discriminator value for tagged union serialization with value `Record(created_at='2024-0...pe='app.bsky.feed.post')` - defaulting to left to right union serialization.
  Failed to get discriminator value for tagged union serialization with value `Main(video=BlobRef(mime_t...='app.bsky.embed.video')` - defaulting to left to right union serialization.
  return self.__pydantic_serializer__.to_json(

i am on

Name: pydantic
Version: 2.9.0

@Meorge
Copy link
Contributor Author

Meorge commented Sep 13, 2024

OOPS TIME TO CHANGE MY PASSWORD Alright, got that taken care of. Maybe a good reason to avoid trying to do this kind of thing first-thing in the morning, haha 😅

@MarshalX I don't get any warnings like that. Using the following test script (now with 100% fewer credentials):

from atproto import Client
from dotenv import load_dotenv
import os

def main() -> None:
    load_dotenv()
    client = Client()
    client.login(os.getenv("USERNAME"), os.getenv("PASSWORD"))
    
    print("About to send video...")
    
    # replace the path to your video file
    with open("video.mp4", "rb") as f:
        vid_data = f.read()
    
    client.send_video(text="just an API test post", video=vid_data, video_alt="this is a video")
    
    print("Post complete")

if __name__ == "__main__":
    main()

My terminal output only shows my print statements:

(.venv) ➜  atproto-send-videos-test python test_send_video.py          
About to send video...
Post complete
(.venv) ➜  atproto-send-videos-test 

And the post itself, with the video attached, is successful: https://bsky.app/profile/meorge.com/post/3l4274csmvr2k

This is with the following versions for pydantic and pydantic_core:

pydantic          2.9.1
pydantic_core     2.23.3

@MarshalX
Copy link
Owner

@Meorge guess what. i've updated pydantic to 2.9.1 (from 2.9.0) and warnings are gone

@MarshalX MarshalX changed the title Add Client.send_video method Add Client.send_video high-level method Sep 13, 2024
@MarshalX MarshalX merged commit 9668e5f into MarshalX:main Sep 13, 2024
20 checks passed
@MarshalX
Copy link
Owner

thank you for your contribution!

@Meorge
Copy link
Contributor Author

Meorge commented Sep 13, 2024

No problem, thank you for testing and accepting it! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Client.send_video method
3 participants