search

matplotlib / pytest-mpl

A pytest plugin to facilitate image comparison for Matplotlib figures
Other
244 stars 47 forks source link

decorator hash_library kwarg option precedence #154

Open bjlittle opened 2 years ago

bjlittle commented 2 years ago

Within the plugin.ImageComparison.compare_image_to_hash_library method, should it not be the case that the mpl_image_compare marker hash_library kwarg option should have precedence over the --mpl-hash-library CLI argument?

https://github.com/matplotlib/pytest-mpl/blob/e3876180847c2d04aeda1982372074998a7f8716/pytest_mpl/plugin.py#L520-L521

At the moment this isn't the case, however a similar precedence is supported for the baseline_dir decorator option e.g.,

image

bjlittle commented 2 years ago

I'm happy to take this on, if this is the case :+1:

Cadair commented 2 years ago

I am not sure about this one. It seems to me that having the CLI override the one in the code makes more sense, as it would let a user specify a different test hash lib or something?

The fact it's inconsistent is more problematic. I am up for input from others here, @ConorMacBride @astrofrog

ConorMacBride commented 2 years ago

It would be more typical for a CLI argument to override the configured value. However, in this case a different hash_library (and a different baseline_dir) can be passed as a kwarg to each test, with the CLI argument being used as the default for kwargs not passed to a specific test.

So each test, in theory, could use a different hash library, and overriding the configured value would break this. That said, I think using multiple hash libraries is not tested and therefore not encouraged.

If you think we should support a different hash library and baseline directory for each test then I think kwarg should take priority. But if we are to require the same library and directory, I think the CLI argument should take priority.

Cadair commented 2 years ago

This feels like it is walking into a bigger discussion including some of the points raised in #149.

At the moment our API is very flexible in terms of what we test against, defined per-test in Python you can do pretty much anything you like based on installed package versions etc.

I think I would be pro reducing that complexity a little, and that includes one baseline_dir / hash library per test run. It would make it easier to configure (you could set some config option in conftest.py or at the CLI) and reduce the overhead.

That being said, we have provided the per-decorator argument since this package existed (right @astrofrog?) and there are people using it, so any change from that would have to be slow and throw many-a-warning.

bjlittle commented 2 years ago

I guess what would be ideal here is aiming for a consistent behaviour across pytest-mpl, whatever that is... but in order to do that, from what you're saying, that might be a breaking change for a major release.

If that's the case, are there more such changes that can be part of such a major release?

bjlittle commented 2 years ago

For example, if #150 lands, would you guys consider making phash the default kernel instead of sha256?

If so, then that would be a breaking change in behaviour, and that's a suitable change for a major release... 🤔

Cadair commented 2 years ago

I agree consistency is a good goal.

Planning for a major breaking release also sounds like a good idea. I not sure if I will have the time to really sit down and audit the API, but if someone else wants to do it that would be great :D

bjlittle commented 2 years ago

Cool, I'll scrape through the code and attempt to summarise the current state of the API, then we can roll from there 👍

ConorMacBride commented 1 year ago

I agree consistency is a good goal.

Planning for a major breaking release also sounds like a good idea. I not sure if I will have the time to really sit down and audit the API, but if someone else wants to do it that would be great :D

I've reviewed the API in #198 and in #199, and have made some suggestions for changes. Suggested changes relevant to this issue are:

  1. Deprecate the use of multiple hash libraries in the same pytest run (not necessary because files are small and include the full module path anyway).
  2. Deprecate the use of multiple testing modes in the same pytest run (e.g. warn if one test only has hash comparison while the others only have image comparison)
  3. --mpl-hash-library should be relative to where pytest was run (to match --mpl-baseline-path).
  4. Local hash_library kwarg should take precedence over global --mpl-hash-library CLI option. (Should not matter given [1] but good for consistency with the baseline_dir kwarg.)
  5. Add mpl-hash-library ini option (relative to where pytest was run). (not a breaking change)

For the two deprecations, I don't think we should go out of our way to prevent people doing these but we should detect these cases and warn the user that it is not supported. I do think we should support multiple baseline images directories as it can be nice to have a separate baseline image directory within each test module, and this is actually what pytest-mpl does by default currently.

I think it is useful to keep the hash_library decorator even if they all need to have the same value because some packages determine the name of the hash library depending on what version of Matplotlib and FreeType is installed.

Please let me know if you have any thoughts, either here or in #198