Expand an exception catching

[neutrinoceros]

PR Summary

This one try block catches a lot of unrelated stuff so I'm expanding it explicitly as a better signal this should be refactored.

Edit (march 4 2021);
The plan for this PR, as discussed in today's triage meeting is to write an lasting issue for it, then merge the expansion.

note: there's a bug upstream in flake8-bugbear that crashes flake8 with the code I'm writing here (is it that bad ??)
PyCQA/flake8-bugbear#153
I think I'll need to fix this myself at some point
edit: I fixed it here PyCQA/flake8-bugbear#161
edit 2: and the fix is now part of bugbear 20.3.2 🎉

[munkm]

I guess I don't see why we couldn't have this be an issue linking to the try block in the original code, rather than adding all of these exceptions into the codebase?

[neutrinoceros]

An issue might get outdated and make it harder to keep track of the problem. While if the code itself gets outdated however, that is, if a new undue exception happens in future contributions, it will get caught before it's too late. Does this sound reasonable ?
That being said, I want to organise the list and add comments to target only the ones that shouldn't have to be there.

[munkm]

An issue might get outdated and make it harder to keep track of the problem.

I am not sure how an issue talking about how this code could be improved could get outdated? If it gets outdated because the code improves that means it is resolved. If it gets outdated because the code is removed that means it is resolved.

While if the code itself gets outdated however, that is, if a new undue exception happens in future contributions, it will get caught before it's too late.

I am not sure what you mean? Catching a broad exception (like what was done originally) here is clear, and making a list like this makes it likely that we are going to miss some exception that isn't in the list. Then what happens? Also adding code like this that is clearly something that we intend to fix is intentionally adding a maintenance burden to the codebase, rather than to our issue list. We're already limited in our developer time. I don't think we should add code to the codebase that we intend to change like this.

[neutrinoceros]

So for the record, I would agree to switch the PR to draft and then try to handle the issues in-situ over time, but this will be a long standing and I expect it will end up to be pretty complicated to review.
That being said, I’ll try to make it clearer why I’d like it merged as is.

Catching a broad exception (like what was done originally) here is clear

I want respectfully disagree on this point. I guess it is possible that catching each and every exception could make sense, but this is generally to be avoided. I think the list already contains a minimum of 4 exceptions that should not be caught because they signal an actual malfunction rather than a missing field of some sort. Keeping the except Exception: only makes it possible to create even more bugs that will not show up until someone realises their dataset is missing some derived fields that should be included (in particular, this could hide frontends bugs.).
We’re already limited in our developer time. I don’t think we should add code to the codebase that we intend to change like this.
If that needs clarification I volunteer to tackle this down, but it’s precisely because I don’t have infinite time to devote that I’d like to be able to break it down in smaller steps 🙂

[neutrinoceros]

So as concluded in private discussion, I now align with your comment that this could be solved in a single PR, so I'm switching this to draft and will start deactivating/solving stuff here.

[neutrinoceros]

note for the future:
RecursionError can be trigger with e.g.

pytest yt/frontends/eagle/tests/test_outputs.py

given at least one of the two sample dataset is present on disk.
This specific error however is extremely brutal to debug as the stack trace mostly refers to nested functions and it's virtual impossible for me to understand where it's originated.

[neutrinoceros]

@munkm I'm now tempted to close this because it takes forever to solve the multiple problems this is meant to reveal and I don't want this PR to rot away for ages. I still believe it has some value in its current state because of the amount of time I spent expanding it and because merging this would also help reduce the amount of "magic" (and impossible to track down bugs) in the code base ever so slightly. In any case let me know what you think is best, I just don't want to keep this around anymore.

[munkm]

Hmmmm. Ok let me think about it a bit.

[chrishavlin]

LGTM! only one question about the code changes.

More generally, I agree with @munkm that introducing a change as a placeholder to fix later is not ideal but I think it's worth it here. Having the minimal set of exceptions explicitly listed will definitely help break up the work of figuring this out and encourage people to give it a go. I think if we opened an issue and left the general exception it would feel too overwhelming... But we should also have an issue open to remind us of this PR.

[chrishavlin]

is there a reason for using print here instead of mylog?

[munkm]

+1 to this.

[neutrinoceros]

Thank you Chris !
Since the triage I updated the description: because this PR triggers a bug in flake8-bugbear, I want to solve that bug upstream first.
I will open an issue here and update this PR before it can get merged !

[munkm]

After a conversation in the triage meeting last week I agree that this can be merged as long as we have a companion issue that links to the lines of code where all of these exceptions are added (in check_derived_fields).

[munkm]

+1 to this.

[neutrinoceros]

Oh no... too late. This is now failing because we let some other PRs in that introduce RecursionErrors that we are not catching here 😢
I'll add it back to the greylist with a note that it is most definitely a bug.
Also @munkm , I just remembered I already have an issue to keep track of the problem after this is merged (#2853). I'll reference it directly in the code.