refactor: python examples log level overwrite mechanism

[andiwand]

I noticed that some python add* functions provide a mechanism to overwrite the logLevel but others don't which made me change reconstruction.py locally from time to time

I tried to generalize the existing functionality and extract it into a helper function. then I applied it to all the add* functions

after some discussion I removed dump_args_calls from the add* functions as this can have side effects and break the CI

[codecov[bot]]

Codecov Report

Merging #1448 (aa0a163) into main (f3ea581) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1448   +/-   ##
=======================================
  Coverage   48.59%   48.59%           
=======================================
  Files         381      381           
  Lines       20631    20631           
  Branches     9463     9463           
=======================================
  Hits        10026    10026           
  Misses       4066     4066           
  Partials     6539     6539           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[andiwand]

Thanks for this. It will be good to have a consistent interface. To that end, can you do the same for itk.py and odd.py, where relevant? Those have the potential to show a lot more detail, so we need to be sure to keep the default the same.

I will take a look. the interesting thing there is that we do not have a sequencer which needs further generalization of the refactor

One problem is that the logic used to select between the different settings (s.config.logLevel, add*(logLevel), and customLogLevel(custom)) is confusing and often doesn't do what we want. The original idea of the custom level was to allow different levels for some algorithms to match the original implementation (before we moved to add*()).

Note that max(custom.value, logLevel.value) chooses the least verbose of the two, so you can only make it less verbose than the default (usually INFO) with the add*(logLevel) argument.

I feel like custom should always overrule if present - if not present we use add*(logLevel) - if not present we use .config.logLevel. Would that make sense?

I don't think this PR changes the logic, so maybe we can accept it and leave improving the logic till later. Or should we try to fix it all now?

I am happy to include it if we can agree on something. otherwise I would go with a pure refactor that does not change the behavior first and later we can change the rest

[andiwand]

@timadye I noticed that acts.examples.dump_args_calls(locals()) will fail the CI somehow when multiple pytests run in a row. this didn't happen before because it was not called. do you know how this could happen and how to fix it? I will also try to look into this tomorrow

[timadye]

@timadye I noticed that acts.examples.dump_args_calls(locals()) will fail the CI somehow when multiple pytests run in a row. this didn't happen before because it was not called. do you know how this could happen and how to fix it? I will also try to look into this tomorrow

Hi @andiwand, I never used that in the CI, just for debugging. I guess it's good to to also test dump_args_calls in the CI, so we should try to fix it. Where is it called?

[andiwand]

@timadye I noticed that acts.examples.dump_args_calls(locals()) will fail the CI somehow when multiple pytests run in a row. this didn't happen before because it was not called. do you know how this could happen and how to fix it? I will also try to look into this tomorrow

Hi @andiwand, I never used that in the CI, just for debugging. I guess it's good to to also test dump_args_calls in the CI, so we should try to fix it. Where is it called?

this is one of the CI jobs then fails https://github.com/acts-project/acts/runs/7975219427?check_suite_focus=true#step:8:4748. I tried to isolate this a little and it looks like removing the if here and just going with

acts.examples.dump_args_calls(locals())

will fail pytest Examples/Python/tests/test_examples.py

maybe this is due to having multiple dump_args_calls calls in a single process which somehow stacks up to an error. because running the tests that fail separately with -k will interestingly not cause a failure @timadye

[andiwand]

Hey @timadye ! I think I know what is going on: the problem is that after wrapping isinstance will not work anymore and this is used quite a lot in the python unit tests. Before my changes the wrapping was not triggered in the unit test context but now they are and this is where it breaks.

Do you think it makes sense to move the wrapping out of add* as this has some side effects? Is it possible to have the call in the main script for example?

Another alternative might be to flag the call explicitly with a named argument in all the add* functions?

[timadye]

the problem is that after wrapping isinstance will not work anymore and this is used quite a lot in the python unit tests.

Yes, isinstance checks for wrapped classes won't work, unless you use inspect.unwrap, eg.

isinstance(mdecorator, inspect.unwrap(acts.IMaterialDecorator)

That's safe to do, whether or not the class is wrapped.

I only see a couple of isinstance() calls in test_examples.py, so it might be good to protect them. Does that help?

Do you think it makes sense to move the wrapping out of add* as this has some side effects? Is it possible to have the call in the main script for example?

That's a good idea. Even if we can get pytest working with dump_args_calls, I think it would be still simpler not to use it during the testing. It's probably best to leave the dump_args_calls setting to the main program. The full_chain tests have them commented out.

[andiwand]

great then this should be ready @timadye if you want to have another look at the changes!

[paulgessinger]

This seems like a good idea to me. Can we get this in @andiwand @timadye?

[andiwand]

This seems like a good idea to me. Can we get this in @andiwand @timadye?

if you could take another look @timadye that would be great! from my side it looks okay now

[timadye]

Otherwise looks good to me. When we have resolved the question about minLevel and maxLevel (just docs, I think), I think we can approve. Should I press the button?

[acts-project-service]

The backport to develop/v19.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-develop/v19.x develop/v19.x
# Navigate to the new working tree
cd .worktrees/backport-develop/v19.x
# Create a new branch
git switch --create backport-1448-to-develop/v19.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 433a379bf53912a5ef3d515ebb23f2bf3d1af331
# Push it to GitHub
git push --set-upstream origin backport-1448-to-develop/v19.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-develop/v19.x

Then, create a pull request where the base branch is develop/v19.x and the compare/head branch is backport-1448-to-develop/v19.x.