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

decorator hash_library kwarg option precedence #154

Open
bjlittle opened this issue May 19, 2022 · 9 comments
Open

decorator hash_library kwarg option precedence #154

bjlittle opened this issue May 19, 2022 · 9 comments
Milestone

Comments

@bjlittle
Copy link
Contributor

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?

hash_library_filename = self.hash_library or compare.kwargs.get('hash_library', None)
hash_library_filename = (Path(item.fspath).parent / hash_library_filename).absolute()

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

image

@bjlittle
Copy link
Contributor Author

I'm happy to take this on, if this is the case 👍

@Cadair
Copy link
Contributor

Cadair commented May 20, 2022

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
Copy link
Member

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
Copy link
Contributor

Cadair commented May 20, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

Cadair commented May 20, 2022

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
Copy link
Contributor Author

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

@bjlittle bjlittle mentioned this issue May 20, 2022
16 tasks
@ConorMacBride ConorMacBride mentioned this issue Apr 9, 2023
22 tasks
@ConorMacBride
Copy link
Member

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

@ConorMacBride ConorMacBride added this to the 1.0.0 milestone Apr 9, 2023
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

No branches or pull requests

3 participants