Let the user know there could've been some problems with the indexing #1083
Conversation
scarlehoff
changed the title
scarlehoff
force-pushed
the
check_fit_indexed
branch
from
5351c34
to
9688c03
Compare
|
As luck would have it, all the fits and pdfs I've uploaded to test this were indexed so it's hard to know whether it would work if they weren't. Other than that this is ready for review. As a side note, I think the pdf and fit uploader classes could be further merged. |
| return True | ||
| return False | ||
|
|
||
| def _check_existence(self, fit_name): |
There was a problem hiding this comment.
Because I felt it was ugly to have basically the same code copied twice for pdfs and fits but at the same time I didn't want to remove the duplicated code that already existed.
Possibly. I would be happy if that was done. |
|
Just to add a note the remote fits are cached so one should not expect them to be updated by repeated calls. This matters because I was about to suggest waiting for a shorter period of time and then retrying, which may still be a good idea but has to be done carefully. nnpdf/validphys2/src/validphys/loader.py Line 696 in ab97f16 |
Ok, I'll do this since I'm already here. |
|
Ok, done. The second commit (cb19c0b) might be a bit more controversial of course. On the bright side I got an "index failure" (and it was true, I uploaded again and this time it worked) so I was able to test that as well. |
|
If we are doing OOP here we could as well do it all the way. Rather than having a weird inheritance chain of PDFUploader(FitUploader) we should have an intermediate class ArchiveUploader that encapsulated the common behaviour. |
|
Yes, that's fine. Although in this case the pdf does work as a subclass of fit. I'll do it tomorrow if I have time. |
|
I've used your |
| raise UploadError | ||
|
|
||
| def compress(self, output_path): | ||
| log.info("Checking whether %s was correctly uploaded...", resource_name) |
There was a problem hiding this comment.
Is there a reason why you don't use an f-string here and in one other place below? Personally I tend to find them a bit easier to read
There was a problem hiding this comment.
Because has bullied me for years.
In reality I guess if you are ever bottle-necked by the writing of the logging then you have bigger problems than that... tbh I don't mind either way.
There was a problem hiding this comment.
I think that you should really use this format in logging right? pylint complains if you don't, something about lazy formatting so I guess this gets resolved later than the f-string?
There was a problem hiding this comment.
see: https://stackoverflow.com/a/49884004 for example
There was a problem hiding this comment.
Yes, that's the case. But tbh I don't think "compiling" a string will ever be a bottleneck of any kind...
There was a problem hiding this comment.
Sure sure, I was just saying I think we should follow the "rules"
There was a problem hiding this comment.
I think the more valid concern is that logging has the very theoretical capability of doing things with the args other than printing them. But it is also unlikely to matter.
Co-authored-by: Cameron Voisey <32741139+voisey@users.noreply.github.com>
|
I'm not sure how best to check whether you get the right behaviour when it doesn't index properly...? I've uploaded a test fit which worked fine, so that's not broken at least... |
|
I don't know. I got one that failed and actually failed but other than luck I'm not sure how to do it without xluttering the server |
|
Shall we just merge then? |
|
Tested and looks good to me, thanks! |
Related to the discussion yesterday.
Before discussing how to best re-index or re-upload I think it is important that at least the code doesn't give the wrong message, i.e., if don't see any errors in your terminal it should really mean there were no errors.
So this just checks whether the fit was indexed.
This is a prototype but before adding the to-dos below (which are mostly cosmetic) I would like whether you think it is worth it or not @Zaharid @scarrazza
To do:
-Control the wait time
-Add a command line argument to skip the wait
-Follow the structure of the rest of the class instead of just having a chunk of code there in the middle.
While this doesn't truly solve #1079 it makes it less of a problem.