Improve downloader interface#59
Conversation
roygutg
left a comment
roygutg
left a comment
There was a problem hiding this comment.
All changes to Downloader look good!
But notice that it's still running write_csv as part of the download method. As you wrote in TODO, that can be removed. That would require a change in the test_downloader function in test_behavioral_pipeline.py: either adding a call to dump after downloading, or assigning the return value of download directly to the raw variable, instead of its current definition. Only then we can make sure that the new output formatting passes the downloader tests.
Additionally, it's finally time to delete the long TODO under create_output, in current lines 142-147. Please do that :)
Regarding the changes to Parser, it seems to not be working as expected. test_parser in test_behavioral_pipeline.py is failing for me (running remotely on lab linux). From a quick look, it seems like all shape IDs parse as None, and all shapes have a gallery save time for some reason. Please look into that.
Thanks!
| output_file_writer.writerow(output_json_record) | ||
|
|
||
| if verbose: | ||
| print(f"Wrote CSV to {self.output_filename}") |
There was a problem hiding this comment.
this can go in the dump method
There was a problem hiding this comment.
HI,
Sorry for the delay on this.
Parser tests pass smoothly now :)
Downloader tests are still not, because the output is not saved (should be either dumped and read, or assigned to raw - see my last comment on that).
Also, lmk what you think about the remaining unresolved comments.
Thanks!
|
No worries, thank you for reviewing. I ran all tests - I thought I fixed the downloader tests, I must have just not pushed those changes by mistake because everything was running fine by me. |
|
I will look at the other comments tomorrow, I seem to have missed them. |
roygutg
left a comment
roygutg
left a comment
There was a problem hiding this comment.
All tests pass now on my side too :)
But I don't see the changes relating to the other comments. Maybe still missing some commits?
|
I was just waiting to run the tests and haven't had a chance - they take like 2 and a half hours |
roygutg
left a comment
roygutg
left a comment
There was a problem hiding this comment.
Hey,
Again, sorry for the long delay!
I still don't see the changes to the dump method mentioned in the comments, but that's not imperative.
All tests are still working fine on my side, and none of the remaining issues is crucial. Given my availability constraints, I think it's best to just be done with this CR :)
If you want, you can address the remaining comments before merging.
To finish this up, please use the GitHub web interface to squash and merge. Then, once indicated "safe", delete this branch. Lastly, please go to #14 and close it with a comment saying it was resolved by this PR (with a link back here).
Thank you!
| @@ -1,4 +1,5 @@ | |||
| import csv | |||
| output_json_record[self.config.RAW_PLAYER_CUSTOM_DATA] = player.get("customData") | ||
| return output_json_record | ||
|
|
||
| def _create_df(self, *, output_json: dict, verbose: Optional[bool] = False) -> pd.DataFrame: |
There was a problem hiding this comment.
this is called from create_downloader_output with output_json as a type list[dict]
|
Hi Roy, |
2 participants
No description provided.