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

V2.0 update for code & documentation #2024

Merged
merged 84 commits into from
Nov 20, 2024
Merged

V2.0 update for code & documentation #2024

merged 84 commits into from
Nov 20, 2024

Conversation

OsaAjani
Copy link
Collaborator

@OsaAjani OsaAjani commented Aug 8, 2023

  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
  • I have properly documented new or changed features in the documentation or in the docstrings
  • I have properly explained unusual or unexpected code in the comments around it

This is it, the candidate PR for v2.0 release as mentioned in issues #1874, #2012 and #1089, with both code update and documentation update.

I wont go in details over all the changes in this PR as they're too many, but roughly, for MoviePy itself:

  • Update of all effects to use class instead of simple functions
  • Improve consistency in the entire API and especially for clip methods naming
  • Fixing a few bugs here and there
  • Getting rid of many dependencies (scikit, imagemagick, opencv, etc.) and reducing image manipulation lib to only using pillow
  • Updating many dependencies to more recent versions
  • Complete suppression of moviepy.editor and runtime magic such as auto-adding of effects as clip methods
  • A complete refactoring of clip previewing to replace pygame and other tools with only ffplay
  • Fixing and updating a lot of in-code documentation
  • Lot of unit test fixing and updating to follow the new API
  • And probably a lot more things I'm forgetting

For the documentation:

  • Update to a new sphinx theme with PyData theme
  • A total restructuring of the documentation to be in 4 parts, getting started, user guide, developer guide and API manual
  • Writing of a 10 minute introduction tutorial to moviepy
  • More logical and chronological organization of the previous "user guide"
  • Lot of general updating to follow the new API
  • Lot of examples writing with more realistic scenarios and full testing on doc examples
  • Improvement of the API doc auto-generation
  • And again a lot more things I'm forgetting

The documentation certainly need to be improved and proof-readed. I'm sure it's pretty obvious from my english, but I'm not a native speaker, and I'm sure that the documentation will be full of grammatical errors, typos, and down right atrocious writing (and because writing doc is such a tedious and forth and back process, I'm sure there is also many logical error, forgotten sentences, unclear explanations, etc.). So I would strongly advise for at least a superficial proof-reading by a native speaker. If not for substance, at least for form.

I'm sure some old bugs still exists, but everything is passing the unit tests, and most bugs should probably be easily addressable along the road. So, I think this PR should be merged as soon as possible (I would say as soon as the first proof-read is done), the documentation updated (keeping the old doc accessible under something like /v1 would be good though), and the PyPi module updated, then we can see what we want to do for the future.

osaajani added 30 commits August 1, 2023 01:48
…llow for text clip. Update setup and some test accordingly.
…ile, as its just a tools to create a composite video clip, just like clip_array
…ge dependency, simplifying resize but removing segmenting
…e now simple effects and have been migrated from function to class + fix a missing @DataClass in FadeOut. moviepy.video.compositing.transitions as well as transfx have been removed
@paulocoutinhox
Copy link

+1

@paulocoutinhox
Copy link

Support PIL 10 pls to solve the bugs

@t4k
Copy link

t4k commented Nov 3, 2023

I see no response to the concern about large media files in the repo. I would like to second those concerns. Including such large files seems like a waste of bandwidth and storage for every person and machine that will use this repo. Links to external videos would probably be fine. I think they should be removed before this PR is accepted.

@OsaAjani
Copy link
Collaborator Author

OsaAjani commented Nov 3, 2023

I see no response to the concern about large media files in the repo. I would like to second those concerns. Including such large files seems like a waste of bandwidth and storage for every person and machine that will use this repo. Links to external videos would probably be fine. I think they should be removed before this PR is accepted.

I do agree it would be best to extract the biggest files from the repo and simply host them somewhere else, maybe just on the web-server used to host the doc.

In fact, there is two different kind of "big files" (comparatively to small text files such as code or doc), the ones used by the tests (mainly anything in /media) and the ones used by the documentation (mainly anything in /docs/_static/medias).

The first type need to stay in the repo somehow (either directly, or with something like git-lfs), because they are used by the unit tests and necessary to ensure there is no regressions.

The second type could totally be hosted anywhere as a simple file to be downloaded. There is almost no point in keeping it in the repository. We just need a place that is cheap as dirt and that we can trust. Git LFS would be quite nice, but it's expensive as f*ck, unless hosting it ourselves, in which case it is lot less expensive but probably a lot more work to maintain.

The resulting trailer should probably be uploaded to YouTube, like @Zulko did for the current documentation, and the documentation updated accordingly. The only reason I didn't do it is that it should be on a trusted channel, not mine.

The big .zip file cannot be uploaded to YouTube though, so we need a way to host this one, maybe some kind of google drive associated to the YouTube channel or something. I leave it for @Zulko to find how he want to do that.

If you can simply tell me where to point the links at I will update the PR to remove the files from the sources and update the doc.

@seven-dev
Copy link

It would be cool to move this forward. I'm going to start using it locally to see if I see any problems while using it, maybe eventually I can help maintaining it.

@Kaszanas
Copy link

Kaszanas commented Jan 5, 2024

I see no response to the concern about large media files in the repo. I would like to second those concerns. Including such large files seems like a waste of bandwidth and storage for every person and machine that will use this repo. Links to external videos would probably be fine. I think they should be removed before this PR is accepted.

I do agree it would be best to extract the biggest files from the repo and simply host them somewhere else, maybe just on the web-server used to host the doc.

In fact, there is two different kind of "big files" (comparatively to small text files such as code or doc), the ones used by the tests (mainly anything in /media) and the ones used by the documentation (mainly anything in /docs/_static/medias).

The first type need to stay in the repo somehow (either directly, or with something like git-lfs), because they are used by the unit tests and necessary to ensure there is no regressions.

The second type could totally be hosted anywhere as a simple file to be downloaded. There is almost no point in keeping it in the repository. We just need a place that is cheap as dirt and that we can trust. Git LFS would be quite nice, but it's expensive as f*ck, unless hosting it ourselves, in which case it is lot less expensive but probably a lot more work to maintain.

The resulting trailer should probably be uploaded to YouTube, like @Zulko did for the current documentation, and the documentation updated accordingly. The only reason I didn't do it is that it should be on a trusted channel, not mine.

The big .zip file cannot be uploaded to YouTube though, so we need a way to host this one, maybe some kind of google drive associated to the YouTube channel or something. I leave it for @Zulko to find how he want to do that.

If you can simply tell me where to point the links at I will update the PR to remove the files from the sources and update the doc.

I think it is also possible to hold large binary files in a release if uploaded manually. This way you can have a large file download script and check it's expected md5. But that may be breaking other things? I am not sure.

@Paillat-dev
Copy link
Contributor

Paillat-dev commented Mar 1, 2024

@OsaAjani Could you please publish a live version of the updated docs? I would appreciate that as I would like to start to test this, so that I can understand the api changes.

@Paillat-dev
Copy link
Contributor

Paillat-dev commented Mar 4, 2024

Okay I started using this, and for now it looks very good, I don't see any issues. I will add comments If I find any, but for now it really looks good, and, way better than the master &/ v1. I hope this gets merged at some point, I have high hopes on this.

setup.py Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't agree on having this huge media files in the repo. For intance, it will affect CI speed if not taking care of it in during the checkout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I completely agree with this, but we still need to find a solution to store those large files and allow for management if we need to update them. And I just dont have time to figure it out right now. If someone can offer a nice & simple solution I'll be happy to update the code and remove those files from codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean can't we use lfs then have it skip lfs in CI?

docs/conf.py Outdated Show resolved Hide resolved
@B3QL
Copy link
Contributor

B3QL commented Jul 11, 2024

@mgaitan can you take another look? It looks like @OsaAjani resolved all issues.
It would be great to merge this and push the project forward. Thanks!

@Paillat-dev
Copy link
Contributor

I really think that this could help the project regain attraction. This pr is what we need. Have been using it in a prod environment for now more than 4 months and it is a really good improvement.

@Cokral
Copy link

Cokral commented Nov 19, 2024

Is there anything happening here? or is the merge request dead? I am trying to use this as a base for my current project due to the state of the project.

@OsaAjani
Copy link
Collaborator Author

@Zulko it would be nice to finally merge and be done with it don't you think ?

@Zulko Zulko merged commit 9a5a695 into Zulko:master Nov 20, 2024
1 check passed
@Zulko
Copy link
Owner

Zulko commented Nov 20, 2024

Sorry all for being the worst maintainer ever. I finally took a deep breath, fixed the conflicts (easy), and fixed all the checks (in particular stylistic) so the pipeline is back to green. I merged from that branch above, and I release 2.0 (hopefully in a viable form) to PyPI. I'll look into docs deployment next.

Edit: docs are now released. Thank you @OsaAjani 🙏 I have also added automation for package publication (=just make a new release on github and it'll be pushed to PyPI) and docs publishing (every time something gets pushed to master). And the tests are now fully green. All this should lower the bar for maintainers to just accept merge and publish the future fixes and improvements.

Huge thanks to OsaAjani for making this happen, and a big thank you to everyone here who commented, reviewed, and tested.

@paulocoutinhox
Copy link

Thanks @Zulko, great job man!

@OsaAjani
Copy link
Collaborator Author

Big thanks @Zulko, I'm so happy this finally took place and was given the opportunity to give back some work and love the community !
As I previously sayed moviepy is a tremendous tools and an impressive work on your part @Zulko. Even if it took some time to come to fruition, I definitely think giving it some time and efforts is one of the best contribution I was ever able to make to the free software community and I'm really glad I did it.
I'm currently working until January 2025 but after that I'd like to make some time to try updating the core of rendering to introduce some sort of parallelization to improve performances.

@keikoro
Copy link
Collaborator

keikoro commented Dec 4, 2024

Yay, finally!! Thank you so much, @OsaAjani!

I've been so busy in recent weeks/months I couldn't even make time for reading notifications (nevermind labelling issues...) and missed the release. But either way, I'm really happy the changes finally got merged and v2 is now a thing!

@B3QL
Copy link
Contributor

B3QL commented Dec 4, 2024

@OsaAjani KUDOS for the enormous effort in making the 2.0 version happen 🎉

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.