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

Feature download files #101

Merged
merged 4 commits into from
Aug 25, 2018

Conversation

verybadsoldier
Copy link
Contributor

@verybadsoldier verybadsoldier commented Aug 24, 2018

Adds viewing and downloading source and artifact files.

Solves #81 and #91

@verybadsoldier
Copy link
Contributor Author

As a nice to have, the file could have its modification timestamp set correctly, but I think it's fine (unless someone needs it).

Added that in the latest commit. Good point.

My only concern would be whether the file names should really be stored using absolute paths. I had some troubles opening them under Windows (see the double backslash in the path).

Well, I am not entirely happy with it either but I had no good alternative idea yet. One obvious option would be to store all files as a flat structure in the ZIP. Duplicated filenames would have to been handled. Probably like myfile.py and myfile.py (2).
But preserving the original directory structure of the files would generally be nice also.

@verybadsoldier verybadsoldier force-pushed the feature-download_files branch 2 times, most recently from 9a0d84a to 58b8a50 Compare August 24, 2018 19:23
@chovanecm
Copy link
Owner

chovanecm commented Aug 24, 2018

  1. regarding Python 3.5: I am not sure how long Sacredboard should support it. I am quite happy that there not (that many) people reporting incompatibility with Python 2.7. Maybe it's also time to abandon 3.5 compatibility. I don't have enough data to decide :(
  2. regarding paths: It's fine for me with absolute paths but an alternative might be using the experiment's base_dir. But I'm not sure what happens if one of the files were stored outside that base dir. I think we are better off with absolute paths than to mess with such special cases.

@verybadsoldier verybadsoldier force-pushed the feature-download_files branch 2 times, most recently from a0c69b9 to e6dce1d Compare August 24, 2018 19:29
@chovanecm
Copy link
Owner

I will try to work around that failing test_removing_a_run* tests. Something in the testing library must have changed.

@verybadsoldier
Copy link
Contributor Author

I am currently struggling to get the high score on those Travis checks ;-)

@chovanecm
Copy link
Owner

I have marked the failing tests to be skipped because the problem is obviously not in Sacredboard.

file = dao_files.get(file_id)
data = zipfile.ZipInfo(file.filename)
file: gridfs.GridOut = dao_files.get(file_id)
data = zipfile.ZipInfo(file.filename, date_time=file.upload_date.timetuple())
Copy link
Owner

Choose a reason for hiding this comment

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

What if someone writes implementation for a non-mongoDB backend? There would be no gridfs.
Do you think you could wrap the result of the get method to something more generic?
Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean but I think it is generic in a way by seeing it as duck typing. The returned file object should have the attributes filename, upload_date and read(). So another backend would have to return an object that also provides these attributes and it should work.

In any case the variable annotation from file: gridfs.Gridout is gone already (mainly because Python 3.6 but you are right that it would not be generic).

Ofc I can still change it if you want. For example to a dict as it is done in the metricsdao.py.

Copy link
Owner

Choose a reason for hiding this comment

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

OK, I'm fine with duck typing (I didn't realise that timetuple() is a method of datetime).
But I'd like to see it documented somewhere (e.g. as a docstring in the FilesDAO class or as a unit test checking whether the returned object is really a "duck" :-) ) - just to make it clear to other developers who would eventually implement it for another back-end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good point. I added that information to the docstring of the abstract class.

@coveralls
Copy link

coveralls commented Aug 25, 2018

Coverage Status

Coverage decreased (-2.9%) to 76.522% when pulling 273c4d9 on verybadsoldier:feature-download_files into b78c54c on chovanecm:develop.

@chovanecm chovanecm merged commit 7ff1e13 into chovanecm:develop Aug 25, 2018
@verybadsoldier verybadsoldier deleted the feature-download_files branch August 26, 2018 10:50
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