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

Remove overly aggressive exception handling #1490

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

danlamanna
Copy link
Contributor

Related #1489

@satra I think I understand what you're getting at and this PR is what I'm hoping satisfies requirements. I'll recap my understanding of these changes and rationale for confirmation/discussion.

  1. Stop catching ValueErrors when validating assets
    We shouldn't be marking assets invalid for what are likely bugs in our dandi-archive/dandi-schema software. Instead, an exception will be unhandled and thus tracked in Sentry. The asset will be unable to be validated until we intervene to fix the bug. Since we've been masking this error and storing the results, we can say that all ValueErrors that have been caught to date are of the form Metadata version x.y.z is not allowed. Allowed are: x.y.z, .... (Maybe this is related to people uploading with older versions of cli dandi-cli#935).
  2. Stop catching blanket exceptions when aggregating assets summary
    This could also hide genuine bugs. These are currently logged but not alerted, which is problematic. Historically this has resulted in masking this error:
Traceback (most recent call last):                          
    File "/app/dandiapi/api/models/version.py", line 237, in _populate_metadata                             
    summary = aggregate_assets_summary(                     
    File "/app/.heroku/python/lib/python3.10/site-packages/dandischema/metadata.py", line 334, in aggregate_assets_summary                                    
    _add_asset_to_stats(meta, stats)                        
    File "/app/.heroku/python/lib/python3.10/site-packages/dandischema/metadata.py", line 319, in _add_asset_to_stats                                         
    if "nwb" in assetmeta["encodingFormat"]:                
KeyError: 'encodingFormat'                                  

Does this sound right? And if so, is the idea to merge a PR like this and then address #1450?

@satra
Copy link
Member

satra commented Feb 13, 2023

@danlamanna - thank you. i feel #2 is appropriate, but #1 i think should perhaps be caught and reported/flagged to admin's at least.

as far as i can tell those value errors won't happen on published dandisets, if they were run through the same checker. could we do that as an admin side script, just for sanity?

@danlamanna
Copy link
Contributor Author

as far as i can tell those value errors won't happen on published dandisets, if they were run through the same checker. could we do that as an admin side script, just for sanity?

I double checked this and confirmed that published assets revalidate without error.

@danlamanna danlamanna merged commit 288470f into master Feb 21, 2023
@danlamanna danlamanna deleted the stop-masking-genuine-bugs branch February 21, 2023 16:45
@dandibot
Copy link
Member

🚀 PR was released in v0.3.18 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants