search

Princeton-LSI-ResearchComputing / tracebase

Mouse Metabolite Tracing Data Repository for the Rabinowitz Lab
MIT License
4 stars 1 forks source link

Remove errors #934

Closed hepcat72 closed 6 months ago

hepcat72 commented 6 months ago

Summary Change Description

Errors associated with missing data (that is autofilled) are removed when no output study file is supplied and initially hidden otherwise.

This also performs some improvements to the MultiLoadStatus exception class to encapsulate its logic and simplify the validation code.

Affected Issues/Pull Requests

Review Notes

See comments in-line.

Checklist

This pull request will be merged once the following requirements are met. The author and/or reviewers should uncheck any unmet requirements:

hepcat72 commented 6 months ago

Attempting the same test as previously, I get a different exception this time:

Traceback (most recent call last):
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 177, in dispatch
    return super(DataValidationView, self).dispatch(request, *args, **kwargs)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 209, in post
    return self.form_valid(form)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 222, in form_valid
    self.extract_autofill_exceptions(
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 386, in extract_autofill_exceptions
    self.extracted_exceptions[exc_class.__name__][

Exception Type: KeyError at /DataRepo/validate
Exception Value: 'NoSamplesError'

This is an interesting little challenge. It is a use-case that I'm not sure we should support (outside of gracefully handling it), but let me walk you through my thinking in this design...

So, the reason behind the exception is because the sample sheet can't be loaded. It is missing various required values. The sample table loader hasn't been split apart and refactored yet, so it tries to merge the animal and samples sheet on animal name, but there are no animal names, so it ends up being garbage-in, garbage-out.

I know your likely immediate response to this: you think we shouldn't be error checking at this point. That's a debate we can continue to have, but let me just point out that, if you have all the accucor files you want to populate in an initially unpopulated sample sheet, you can do that by submitting all the files at once. There's no reason to add to an otherwise empty sample sheet, iteratively.

While I think that's enough right there to just note this as a minor issue to solve in a subsequent PR and keep moving forward with this review, I will make sure that the key error isn't raised. However, just handling that won't do what I think it is you're expecting: add the samples to the invalid sample sheet. That's a feature I didn't design and thus I think belongs in a separate effort. I suggest, after addressing the key error, that we review the thus-far designed usage:

  1. The user uploads all the accucors/isocorrs they have and get back a sample sheet
  2. The user works on fleshing out the animal/sample sheet
  3. The user produces more accucor files and then submits them with the updated sample file
  4. Go back to step 2.
hepcat72 commented 6 months ago

Wow, I take that back. The keyerror was simpler than I first had imagined. I solved it with a defaultdict. And I actually implemented it smarter than I'd imagined. It does add to the invalid sheet. I hadn't envisioned this use-case, but it actually works! Fix incoming after linting and testing...

hepcat72 commented 6 months ago

OK, the fix is in. Ready for another look.

hepcat72 commented 6 months ago

@lparsons - Sorry, I tried to sneak in a quick improvement, but I broke 2 tests. Should be quick to fix. Hope I didn't interrupt a review!

hepcat72 commented 6 months ago

Fixed.

hepcat72 commented 6 months ago

I'm still getting an exception when uploading a study sheet. I agree that we don't need to support all use cases (specifically one with missing Animals), but we should avoid the 500 error on the server.

Input files: study.xlsx glnfasted2_cor.xlsx

Exception:

Traceback (most recent call last):
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 177, in dispatch
    return super(DataValidationView, self).dispatch(request, *args, **kwargs)
  File "/home/lparsons/miniforge3/envs/tracebase/lib/python3.9/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 209, in post
    return self.form_valid(form)
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 222, in form_valid
    self.extract_autofill_exceptions(
  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 386, in extract_autofill_exceptions
    self.extracted_exceptions[exc_class.__name__][

Exception Type: KeyError at /DataRepo/validate
Exception Value: 'NoSamplesError'

@lparsons Could you double-check that you pulled the latest changes? I cannot reproduce the exception using the same inputs. And besides, this is not what's on line 386:

  File "/home/lparsons/Documents/projects/tracebase/DataRepo/views/upload/validation.py", line 386, in extract_autofill_exceptions
    self.extracted_exceptions[exc_class.__name__][

You must have a stale branch...?

lparsons commented 6 months ago

You must have a stale branch...?

Oops, my bad! I re-fetched things and it seems OK now. Must not have updated everything. Sorry about that. Reviewing now.

hepcat72 commented 6 months ago

Strangely, when I uploaded a study file with three missing sample names, I got back the updated file, but the (hidden) messages say there were 37 sample names added to the file. Not a blocking issue, but a confusing message.

Yeah, that's a side-effect of the fact that the sample sheet isn't in a loadable state. It's not checking if the samples were in the sheet. It checks if they were loaded. The next PR mitigates this by the fact that it determines if the sample sheet is ready for error-checking (for reporting and/or fixing issues other than missing samples), and if it's not, it extracts the existing samples from the study doc itself. It also doesn't yield a status report either (though my idea to make MultiLoadStatus handle more than just load status would then allow me to report other stats/messages).

I did try creating some invalid sample sheets, an unfortunately, they resulted in some unhandled exceptions. I do not think this is a reason to block this PR, but I would suggest that there be some method to handle unexpected Exceptions and return a "nice" error message about being unable to read the file properly, etc. That can be tackled in a future PR.

That's a good point. I did think about that and expressly decided not to worry about it (yet).