-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement example archive importer #1126
Implement example archive importer #1126
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1126 +/- ##
==========================================
- Coverage 72.49% 71.99% -0.50%
==========================================
Files 105 105
Lines 6810 6871 +61
==========================================
+ Hits 4937 4947 +10
- Misses 1873 1924 +51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if archives := response.content.decode("utf-8").strip().split("\n"): | ||
return [(archive, archive.split("-")[0].strip()) for archive in archives] | ||
self.info.value = "No archives found" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will put, in the readme of the aiidalab-qe-examples package, an instruction on how to add a line in the examples_list.txt file, so that this parsing will always work also in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I committed some mods to the examples repo to simplify fetching.
src/aiidalab_qe/common/widgets.py
Outdated
with open(filename, "wb") as f: | ||
for chunk in response.iter_content(chunk_size=8192): | ||
f.write(chunk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This download will happen in the qe-app root folder, but I am not sure it is the recommended way: if the download or import is interrupted (closing the notebook - the case I tried - , or any other reason), the file will stay there because we don't run the delete_archive of the widget. A issue can be that then we have then storage occupied by this file.
@edan-bainglass, do you agree with this? maybe I am missing something in the code.
My suggestion is:
If we do the download and import from a temporary directory, we can automatically skip the need of deleting the file (so the last call, delete_archive
, in all cases (successfull or not). Is there any reason which we cannot do the process in a tempdir?
I suggest to add destination
and source
input arguments to download_archive
and import_archive
, and run them to use a tempdir:
def import_examples(self, _):
if self.logger and self.clear_log_on_import:
self.logger.value = ""
for archive in self.selector.value:
with tempfile.TemporaryDirectory() as tempdir:
if self.download_archive(filename=archive, destination=tempdir):
self.import_archive(filename=archive, source=tempdir)
#self.delete_archive(archive) <--- no need for this
then, the filename
variable in the two methods should be probably changed to be new_filename_or_path = pathlib.Path(destination or source) / <filename>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections. This is great. I forget tempdir exists 😅 Will implement 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I ended up bypassing the download altogether, as verdi archive import
can directly import from a URL! Very nice 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Even better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edan-bainglass, this is great! I just added a comment on the possibility to do the download in a temporary directory, so we avoid unwanted partially downloaded files in the qe-app root folder, in case something goes wrong.
If the change is not needed, we can discuss about it and I will remove the request.
Thanks!
Thanks @mikibonacci. Yes, let's switch to tempdir. Will update the PR. updateSkipped the download in favor of importing directly from the URL 👍 |
4875740
to
4330e4f
Compare
fbc9a10
to
a760aab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edan-bainglass, LGTM!
91dadec
to
deb4660
Compare
Thanks @mikibonacci. I just pushed an update with a link to the calculation history. Please give it a quick review 🙏 Also, if you can please quickly review #1124 (on which this is based) and its |
42e105a
to
bd6acc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edan-bainglass, LGTM!
bd6acc9
to
60e7ec5
Compare
This PR implements a new notebook (linked from the home page via new button) to import example calculations from aiidalab-qe-examples. The notebook includes:
verdi archive import
Future improvements
ArchiveImporter
could be made more general, allowing imports from any URL (MC for example)ArchiveImporter
to AWBCloses #1117