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

Pipeline robustness when reading ETC values from raw data header #1309

Merged
merged 5 commits into from Jun 16, 2021

Conversation

akremin
Copy link
Member

@akremin akremin commented Jun 16, 2021

This PR fixes a corner case where ACTTEFF is defined in the raw data header but is set to an empty (non-numeric) value. This caused the pipeline to crash. Now we test that it can be transformed into a float within a try statement.

This was part of a larger discussion that led to the choice of deprecating ACTTEFF in favor of ETCTEFF, which is also in the raw data header. For future dates on or after 20210614, only ETCTEFF will be checked. The old keyword is kept for compatibility with old data (under an assertion that night<20210614).

I tested this on nights in Dec 2020, Jan 2021, Feb 2021, April 2021, May 2021, and June 14 2021. No issues on any date.

@akremin akremin requested a review from sbailey June 16, 2021 00:22
@coveralls
Copy link

coveralls commented Jun 16, 2021

Coverage Status

Coverage decreased (-0.02%) to 26.511% when pulling 921c032 on header_value_robustness into b484730 on master.

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, though please update try/except blocks to catch the specific exception that you are expecting so that we don't silently "work around" future bugs (e.g. I think ValueError for the float casts, I'm not sure about the what might go wrong with the str() cast). OK to self merge when ready, including merging as-is if you need this for tonight and want to post-facto discuss try/except philosophy :)

@akremin
Copy link
Member Author

akremin commented Jun 16, 2021

Given the need for this for processing tonight, I'll self merge this PR and make the requested changes tomorrow in a new one.

It's a very reasonable (and PEP-8 proper) request. That said, I genuinely do want it to pretend like nothing happened if anything goes wrong in trying to use that keyword. If the keyword can't be used I'd rather it move on with its life with its default value rather than crash the pipeline. But I know there are coding best-practices reasons why that's a bad idea. So maybe we do need to have that philosophical chat :)

@akremin akremin closed this Jun 16, 2021
@akremin akremin reopened this Jun 16, 2021
@akremin akremin merged commit ba95e88 into master Jun 16, 2021
@akremin akremin deleted the header_value_robustness branch June 16, 2021 01:16
@sbailey
Copy link
Contributor

sbailey commented Jun 16, 2021

Fine to be pragmatic for tonight. In another semi-recent case we had something like the equivalent of a new bug causing dat_header to be undefined. Generic try/except blocks that were intended to catch things like missing or malformed keywords were instead cascading through "robustness" logic instead of alerting us early that we had mis-merged and deleted the line that was supposed to be reading the header in the first place. Or something like that. But it reminded me of the general wisdom behind catching specific exceptions unless you really really need it to continue no matter what.

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

Successfully merging this pull request may close these issues.

None yet

3 participants