Migrate to src Layout and use pyproject.toml

[paketb0te]

This implements #85.

Unfortunately the tests did not pass on my machine even before migrating, but at least they fail with the same errors (same amount of pixel diff), so I think / hope that I broke nothing in the process...

I added some optional dependencies (see [project.optional-dependencies] in pyproject.toml) which are only required for development, and can be installed with pip install -e .[dev].

(Updated CONTRIBUTING.md accordingly)

Let me know what you think @initialcommit-io 😃

[initialcommit-io]

Thanks for this!!

Unfortunately the tests did not pass on my machine even before migrating, but at least they fail with the same errors (same amount of pixel diff), so I think / hope that I broke nothing in the process...

Interesting... Did every single test fail and can you let me know what the pixel diff was? I believe it should have printed that output. Can you also let me know some basic details about the machine you're using like OS/version?

I added some optional dependencies (see [project.optional-dependencies] in pyproject.toml) which are only required for development, and can be installed with pip install -e .[dev].

I like that. Since you mentioned PIL (pillow) before, is that something that should be added into those optional dependencies? Looks like I didn't see it there...

Otherwise it looks OK to me! Are there any other changes to our DEV/deployment flow that you can think of that would occur after changing from setup.py to pyproject.toml?

[initialcommit-io]

Also for the failed tests it would be useful to find where it stored the generated images (somewhere in a local temp directory) and see if there are any obvious differences between those and the reference files.

For example one thing that @ehmatthes and I ran into several times are obvious differences in fonts across systems that would cause the pixels to be different enough from the reference images to fail the tests.

[paketb0te]

Since you mentioned PIL (pillow) before, is that something that should be added into those optional dependencies

It is already installed as a dependency of manim, so I did not add it explicitly (but we can do that of course).

Are there any other changes to our DEV/deployment flow

Not that I am aware of... how does your deployment workflow look like? If it's based on setuptools then it should be no problem.

Regarding the failed tests,
I am running Ubuntu 22 and I think you are right about the OS fonts!

When comparing the reference images with the ones generated during testing, it is obvious that the commands are working as expected, but the font is slightly different:

image rendered during test_merge:

reference image:

So I think this is just an artifact of the OS differences.

[paketb0te]

can you let me know what the pixel diff was?

Only one passed, must be revert - it's the only one missing here:

FAILED tests/e2e_tests/test_core_commands.py::test_add - AssertionError: bad pixel ratio (add): 0.022411265432098766
FAILED tests/e2e_tests/test_core_commands.py::test_branch - AssertionError: bad pixel ratio (branch): 0.037940779320987654
FAILED tests/e2e_tests/test_core_commands.py::test_checkout - AssertionError: bad pixel ratio (checkout): 0.03169608410493827
FAILED tests/e2e_tests/test_core_commands.py::test_cherrypick - AssertionError: bad pixel ratio (cherry_pick): 0.03452498070987654
FAILED tests/e2e_tests/test_core_commands.py::test_clean - AssertionError: bad pixel ratio (clean): 0.020872395833333335
FAILED tests/e2e_tests/test_core_commands.py::test_commit - AssertionError: bad pixel ratio (commit): 0.016613136574074074
FAILED tests/e2e_tests/test_core_commands.py::test_log - AssertionError: bad pixel ratio (log): 0.051217206790123454
FAILED tests/e2e_tests/test_core_commands.py::test_merge - AssertionError: bad pixel ratio (merge): 0.033581211419753086
FAILED tests/e2e_tests/test_core_commands.py::test_mv - AssertionError: bad pixel ratio (mv): 0.02470630787037037
FAILED tests/e2e_tests/test_core_commands.py::test_rebase - AssertionError: bad pixel ratio (rebase): 0.03720148533950617
FAILED tests/e2e_tests/test_core_commands.py::test_reset - AssertionError: bad pixel ratio (reset): 0.022719907407407407
FAILED tests/e2e_tests/test_core_commands.py::test_restore - AssertionError: bad pixel ratio (restore): 0.022619598765432097
FAILED tests/e2e_tests/test_core_commands.py::test_rm - AssertionError: bad pixel ratio (rm): 0.024953221450617284
FAILED tests/e2e_tests/test_core_commands.py::test_stash - AssertionError: bad pixel ratio (stash): 0.022184124228395063
FAILED tests/e2e_tests/test_core_commands.py::test_status - AssertionError: bad pixel ratio (status): 0.022081404320987656
FAILED tests/e2e_tests/test_core_commands.py::test_switch - AssertionError: bad pixel ratio (switch): 0.03169608410493827
FAILED tests/e2e_tests/test_core_commands.py::test_tag - AssertionError: bad pixel ratio (tag): 0.03681568287037037

[initialcommit-io]

It is already installed as a dependency of manim, so I did not add it explicitly (but we can do that of course).

Thanks I think it's a good idea because I believe I had to manually install on my end. For some reason it wasn't pulled in for me as a dependency of something else.

Not that I am aware of... how does your deployment workflow look like? If it's based on setuptools then it should be no problem.

Currently the deployment process is only 2 steps, but I do specifically mention the setup.py file... Currently I just run:

$ python setup.py sdist bdist_wheel
$ twine upload --skip-existing dist/*

Which builds the latest version of the package and then uploads it to the PyPI.

I am running Ubuntu 22 and I think you are right about the OS fonts!

Interesting... @ehmatthes and I specifically picked a font to be used in the tests called "Courier New" and hoped it would exist on major OS's. Any way you can check your system to see if this font exists somewhere? Looks like there may also be an issue with bolding, but the font-family itself is def different between the screenshots you linked.

[ehmatthes]

I am most of the way through a demo of using a local font file for testing. It involves building support for paths in the --font flag, but most of the structure to support that is already in place.

I'll share more when I have another hour or two, but if this works it would do away with these cross-platform testing issues, which I believe will always pop up in frustrating ways. I think it would allow nailing down a tight pixel ratio, and any errors would be breaking issues, not just OS-specific differences.

[ehmatthes]

Okay, quick rundown:

• demo work is on the fix_font_tests branch here.
• Manim requires a context manager if you're using a font file. That context is set in base command: src
• The context is used in each command's construct() method: log example
• This gets ugly in the more complex commands, because it's a whole additional level of indentation: rebase example
• But there's probably ways to manage that.
• The main test file does not change.
• The util get_cmd_parts is modified to use the path to the local test font file instead of "Courier New".

Jacob, I think if you run the test suite on this branch it will pass for you. I had to install fonttools; I think that was the only additional dependency. There's still obvious cleanup, but these are the big picture things that would need to be done for consistent tests across OSes.

[ehmatthes]

One major simplification I haven't had time to dig into: Wherever construct() is called, the context manager could be set there. Then I don't think any of the individual command files would need to be changed. 🤦

[ehmatthes]

I can't spend more time on this right now, but I was able to move the font context work to handle_animations() in commands.py. I think that means all the command files would remain unchanged.

I'll work this up into a clean PR and you can evaluate whether it's worth implementing. (I assume a PR is the best way to share an idea, even if the PR itself doesn't end up being the version that's merged in the end?)

[ehmatthes]

Sorry for sharing the exploratory work yesterday, I'm not sure that was helpful. I submitted a much cleaner PR that should address test consistency across systems.

[initialcommit-io]

Hey thanks for these updates @ehmatthes - they were helpful. When @paketb0te ran into test failures I probably should have made a note to move the discussion about that into #55. But it's all good.

[initialcommit-io]

@paketb0te I merged @ehmatthes PR #98 into the dev branch and added fonttools as a dependency in pyproject.toml. Can you pull those changes and try running the tests again on your end and let us know if it works for you?

Also, let me know if you can think of any potential issues with the new src/ layout and the deployment process I mentioned above. If everything works for you I think we can close this one.

[paketb0te]

Can you pull those changes and try running the tests again?

Just did, all tests are passing 🥳

[paketb0te]

let me know if you can think of any potential issues with the new src/ layout and the deployment process

I think you just need to change your build command to python3 -m build, this will create a source distribution and a built distribution just like python setup.py sdist bdist_wheel.

You probably have to install the build package on your system once (pip install build - who would have guessed 😆 )

# previously 'python setup.py sdist bdist_wheel'
python3 -m build
twine upload --skip-existing dist/*

[ehmatthes]

Hi @paketb0te! Here's compare_images(), in tests/e2e_tests/utils.py:

def compare_images(path_gen, path_ref):
    ...
    # Images are similar if only a small % of pixels differ significantly.
    #   This value can be increased if tests are failing when they shouldn't.
    #   It can be decreased if tests are passing when they shouldn't.
    msg = f"bad pixel ratio ({path_ref.stem[8:]}): {ratio_diff}"
    assert ratio_diff < 0.015, msg

Can you drop the threshold for ratio_diff down to 0.0075 and see if tests still pass for you? How about 0.005? Assuming they fail there, what's the highest failing ratio that you see?

I'm asking because I think we can drop that ratio back to 0.0075 now that everyone's tests should be running with the exact same font.

[paketb0te]

I dropped it down to 0.0075 as you suggested @ehmatthes , but this made 11/18 tests fail again:

FAILED tests/e2e_tests/test_core_commands.py::test_branch - AssertionError: bad pixel ratio (branch): 0.009318094135802469
FAILED tests/e2e_tests/test_core_commands.py::test_checkout - AssertionError: bad pixel ratio (checkout): 0.00876929012345679
FAILED tests/e2e_tests/test_core_commands.py::test_cherrypick - AssertionError: bad pixel ratio (cherry_pick): 0.009206211419753087
FAILED tests/e2e_tests/test_core_commands.py::test_commit - AssertionError: bad pixel ratio (commit): 0.009812403549382715
FAILED tests/e2e_tests/test_core_commands.py::test_log - AssertionError: bad pixel ratio (log): 0.013791473765432098
FAILED tests/e2e_tests/test_core_commands.py::test_merge - AssertionError: bad pixel ratio (merge): 0.008062789351851852
FAILED tests/e2e_tests/test_core_commands.py::test_rebase - AssertionError: bad pixel ratio (rebase): 0.01101707175925926
FAILED tests/e2e_tests/test_core_commands.py::test_reset - AssertionError: bad pixel ratio (reset): 0.008433641975308642
FAILED tests/e2e_tests/test_core_commands.py::test_rm - AssertionError: bad pixel ratio (rm): 0.007954764660493828
FAILED tests/e2e_tests/test_core_commands.py::test_switch - AssertionError: bad pixel ratio (switch): 0.00876929012345679
FAILED tests/e2e_tests/test_core_commands.py::test_tag - AssertionError: bad pixel ratio (tag): 0.009516782407407408

[ehmatthes]

Okay, thanks. I'll set up an Ubuntu VM at some point soon and look at what's happening. I think keeping it at 0.015 is fine for now.

We will almost certainly use a different font for testing long term, and we may change the way the images are compared, both of which would take the place of this setting anyway.