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

Repo activity and pending PRs #382

Closed
mbeacom opened this issue Jan 9, 2017 · 23 comments
Closed

Repo activity and pending PRs #382

mbeacom opened this issue Jan 9, 2017 · 23 comments
Labels
question Questions regarding functionality, usage

Comments

@mbeacom
Copy link
Collaborator

mbeacom commented Jan 9, 2017

Hello!

First off, thank you for the repo @Zulko!

Looking at the repository, it appears there are a good number of pull requests that are awaiting review and merging.

Would it be possible to have some of these PRs merging into master or even a develop branch? Are you open to allowing contributor merging and/or automated builds?

Thank you again!

@Zulko
Copy link
Owner

Zulko commented Jan 15, 2017

Hi @mbeacom,

Thanks for the push. I have spent my programming efforts on new projects in the last year or so, and I have not been doing a great job as a Moviepy maintainer, sorry. I certainly won't have more free time any time soon, so I am open to let contributors merge to the main repo (and maybe later turn MoviePy into a more collaborative project, I'll think about it), as long as the code the quality is ok (pep8) and it conserves py2/py3/linux/mac/windows compatibility and retrocompatibility.

I assume you want merging rights? Are any other regular Moviepy user following this thread interested ?

Thanks for your interest !

@tyarkoni
Copy link
Contributor

I'm new to the package, but would emphatically +1 the suggestion to add collaborators. I can totally appreciate the huge amount of work it takes to keep maintaining a package like this, but given the enormous userbase and other people's (e.g., me!) reliance on the codebase, it would be really great to have regular releases and faster code review. E.g., I submitted a PR (#384) that fixes a completely trivial problem that causes moviepy to outright break on the latest numpy, and this feels like the kind of thing that should be merged into master (and ideally pushed to PyPI) as quickly as possible.

@mbeacom
Copy link
Collaborator Author

mbeacom commented Jan 26, 2017

Thank you for your response, @Zulko!
No worries. You contributed the repo after all.

I would be interested in merging/collaborator access, if you're open to the idea.

@mbeacom
Copy link
Collaborator Author

mbeacom commented Jan 30, 2017

@Zulko Thank you for granting me collaborator access. Would it be possible for you to add me as a PyPi contributor so that I can push updates as well? My PyPi username is: mbeacom

Also, if you need to reach out to me with any questions or concerns, feel free to send me a message on twitter! @mbeacom

Thank you again!

@mbeacom mbeacom added the question Questions regarding functionality, usage label Jan 30, 2017
@Zulko
Copy link
Owner

Zulko commented Jan 30, 2017

@mbeacom Thanks for the help ! I have added you as a maintainer of Moviepy on PyPi. If you're interested, maybe add a "Maintainers" section in the readme with your id and mine ?

@mbeacom
Copy link
Collaborator Author

mbeacom commented Jan 31, 2017

@Zulko I've added the maintainer section to the README. What are your thoughts on automated builds and testing with something like travis? We'll probably want something in place while we move forward with some of the existing PRs. Then comes test coverage! 😀

@Zulko
Copy link
Owner

Zulko commented Jan 31, 2017

@mbeacom Definitely ! Lack of a test suite is the main reason it's difficult to maintain the repo with the little time I have. I cant help setting it up but I can help with writing tests.

However making a test suite for moviepy, like for other media libraries, won't be trivial.

A naive way would be to just run different scripts to cover all features, without checking the output, just to make sure nothing is broken.

If we want to do more, the standard approach would be to compare the library output (images, videos, audio) with pre-generated media, that's what Matplotlib does and unless I'm mistaken that's why their repo is very heavy: because of all the PNGs they need to compare with the many possible outputs of the library. In our case it would be videos, so possibly even bigger.

In addition Moviepy's output doesn't just depend on the code: it is supposed to run on different platforms and can use different versions of ffmpeg which in turn may use different codecs depending on what users have installed. So the comparative test-suite approach may break on some people's machines.

Thoughts ?

@tyarkoni
Copy link
Contributor

Some coverage is better than no coverage, so I think it's fine to put off worrying about different platforms for now, and initially just focus on making sure that the core code runs okay under ideal situations—e.g., on travis or circle. I have a package (pliers) that relies extensively on moviepy, and we use travis for testing, so I've had to work through some ffmpeg installation issues. If it's any use, @mbeacom, you might be able to adapt from my .travis.yml if you end up using travis-ci, which installs and runs moviepy fine (though there do appear to be some moviepy bugs related to ffmpeg that generate errors similar to #281).

@mbeacom
Copy link
Collaborator Author

mbeacom commented Feb 3, 2017

I agree that we should begin with some kind of standard testing suite and worry about platform specific testing later. Regarding sample testing, we can probably move into that. I've seen other repositories use secondary storage for some test files.

@Zulko In the meantime, can you go on https://travis-ci.org and enable the repository and web hook? Once it's enabled, collaborators can do pretty much everything else to get the builds going.

@tyarkoni I was thinking of start out with a matrix running against 2.7, 3.5, and 3.6. We'll circle back to this once the repo is hooked up to travis!

Thanks!

@Zulko
Copy link
Owner

Zulko commented Feb 3, 2017

@mbeacom I activated Moviepy on travis. Do I have to do anything in particular for you to receive build notifications ?

@ghost
Copy link

ghost commented Feb 10, 2017

instead of hosting videos and images in the repo, we could write scripts to download them from some other location, in which we would have control of.

@ghost
Copy link

ghost commented Feb 10, 2017

@Zulko do you have the media files used in the examples folder? Those would make good test cases, since you have put the work into them already.

@keikoro
Copy link
Collaborator

keikoro commented Feb 14, 2017

Subscribing myself to this thread.

Btw. I remember there was someone else a few months ago who thought tests would be good idea. I'm not sure though if they meant to say they could help with setting up tests or if they just liked the idea in general, but I could go and look for their name in the issues.

@keikoro
Copy link
Collaborator

keikoro commented Feb 14, 2017

Btw.2 @mbeacom, I think you mixed up the Markdown formatting for links in the README update! :P

@keikoro
Copy link
Collaborator

keikoro commented Feb 14, 2017

I think @flothesof was who I was thinking of. (^ See my message two comments above this one.)

@ghost
Copy link

ghost commented Feb 15, 2017

I have an idea.. For tests at least for the ones dealing with ffmpeg (external tools), we could have a test function that returns the command line parameters that would be passed to ffmpeg. In a unittest we could see if the results from the test function matches a string, etc.

For example, the FFMPEG_VideoWriter class could contain a test (or some other name) function, that builds the args we pass to Popen. If these args do not change between tests, then we assume the test as successful. an issue I see with this is the value in get_setting("FFMPEG_BINARY"), but we could remove this from the test function and only look at the args passed to ffmpeg.

What do others think?

@takosuke
Copy link

Hi everybody,
been lurking around this repo for ages wanted to contribute and the surge of activity (my email inbox is full of messages about it now!) has made me want to finally get my shit on. I'm going to have a good look at the issues and everything but I'd appreciate any general pointers of where more hands are needed. I'm pretty solid with python, ffmpeg, imagemagick, git and django but testing is a foreign planet to me, if that helps.

@keikoro
Copy link
Collaborator

keikoro commented Feb 17, 2017

@takosuke I haven't had time yet to take a closer look at the (newly) active issues – or old ones – either, but I'm planning to do so over the weekend.

There's no agreed-upon workflow so far, but GitHub just released a new project, Open Source Guides, (which might be helpful in general), via which I found out about this blog post by @steveklabnik, How to be an open source gardener, which presents a helpful approach regarding the tackling of (many) open issues of a project.

@takosuke
Copy link

takosuke commented Feb 17, 2017 via email

@keikoro
Copy link
Collaborator

keikoro commented Feb 17, 2017

@takosuke As said, no workflow has officially been established for this project yet (as far as I'm aware), but I was thinking of following Steve's advice – he provided a step-by-step/breakdown of what he did for (iirc) Rails.

Would be interesting though to hear what the others think about how he went about it. I have no idea if issues/PRs are being approached in any structured way by the others right now.

@gyglim
Copy link
Contributor

gyglim commented Mar 14, 2017

I am a bit late, but I am also interested in becoming a contributor. We are using moviepy in our production pipeline and are thus interested in fixing bugs and writing tests, etc.

@Overdrivr
Copy link
Collaborator

@Zulko I would be interested too in becoming a maintainer of this repository, I have a solid experience in building DevOps pipelines and automating release processes. I can commit some significant time in the coming weeks to setup Windows-based CI + review/merge some PRs. I would be a shame to leave this library unmaintained as there are no other alternative and it does already quite a lot :)

@Zulko
Copy link
Owner

Zulko commented Mar 24, 2019

@Overdrivr Thanks very much for offering your help, I sent you an invite to become a repo maintainer (please ask before merging opinionated or non-trivial PRs, I'll try to be reactive).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Questions regarding functionality, usage
Projects
None yet
Development

No branches or pull requests

8 participants