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

Adding the download button for QeAppWorkChains. #850

Merged
merged 23 commits into from
Oct 21, 2024

Conversation

mikibonacci
Copy link
Member

@mikibonacci mikibonacci commented Oct 8, 2024

This fixes #823

We provide a DownloadDataWidget to download, separately:

  • AiiDA archive: export_{node.pk}.aiida
  • AiiDA raw data export_{node.pk}_raw.zip

both are zip files.

we provide a DownloadDataWidget to download, separately:

- AiiDA archive: export_{node.pk}.aiida
- AiiDA raw data export_{node.pk}_raw.zip

both are zip files.
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 39.13043% with 42 lines in your changes missing coverage. Please review.

Project coverage is 67.10%. Comparing base (23c3fe1) to head (150d805).
Report is 122 commits behind head on main.

Files with missing lines Patch % Lines
src/aiidalab_qe/app/result/utils/download_data.py 37.31% 42 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #850      +/-   ##
==========================================
- Coverage   67.53%   67.10%   -0.43%     
==========================================
  Files          50       51       +1     
  Lines        4620     4688      +68     
==========================================
+ Hits         3120     3146      +26     
- Misses       1500     1542      +42     
Flag Coverage Δ
python-3.11 67.10% <39.13%> (-0.43%) ⬇️
python-3.9 67.13% <39.13%> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mikibonacci mikibonacci marked this pull request as ready for review October 8, 2024 14:49
@mikibonacci
Copy link
Member Author

Hi @superstar54 and @AndresOrtegaGuerrero , this PR is ready for review.

The main new thing is the DownloadDataWidget, in the new utils subfolder of the result one. I left mostly untouched the implementation of a download output button found in the WorkChainOutputs, as I have not fully understood how it works. I preferred to implement a widget which contains both archive and raw_data download buttons.

Files are downloaded as zip archives.

A part the implementation, which probably needs to be improved, only one issue: with google chrome the downloads are seen as insecure (I think because they are zip files, and we have http instead of https).
Could you please confirm that also you have this issue?

Thanks!

@AndresOrtegaGuerrero
Copy link
Member

@mikibonacci thank you , do you think we can include the import aiida node , is too much for this PR ?

@mikibonacci
Copy link
Member Author

Hi @AndresOrtegaGuerrero , thanks for looking at this PR. for the import button, I think I can do a second PR so we don't add too much to this one. But also, I am a bit concerned on what the user may try to upload... for example, maybe they expect that if a PwBaseWorkChain is imported, this will be visible in the app... but this is not always true (true only if we import the whole QeAppWorkChain). Actually, we should be safe enough, as the user typically will just export and import using the app buttons.

To summarize, I think we can do it in another PR, maybe not a critical one. We have already the upload widget from euphonic vibro-app, so should be easy to use similar code and just play with the import API of AiiDA

mikibonacci and others added 4 commits October 9, 2024 14:04
- only one `_download_data` method, which allow to download raw and .aiida data (once at the time).
- QeAppWorkChain node is reloaded when the bitestream creation is done, this is needed to remain in the same Session (otherwise SQLAlchemy will complain that the Db request is not in the same session of the notebook.
@mikibonacci
Copy link
Member Author

Hi @AndresOrtegaGuerrero , I addressed the docstring and the download methods merging. I also put a reloading of the workchain node in the produce_bitestream method, as I encountered a problem with SQLAlchemy (apparently if I don't download again the node, the session in which I produce the files is different from the one of the notebook, and then it does not work - still <100% clear to me)

@AndresOrtegaGuerrero
Copy link
Member

Hi @superstar54 and @AndresOrtegaGuerrero , this PR is ready for review.

The main new thing is the DownloadDataWidget, in the new utils subfolder of the result one. I left mostly untouched the implementation of a download output button found in the WorkChainOutputs, as I have not fully understood how it works. I preferred to implement a widget which contains both archive and raw_data download buttons.

Files are downloaded as zip archives.

A part the implementation, which probably needs to be improved, only one issue: with google chrome the downloads are seen as insecure (I think because they are zip files, and we have http instead of https). Could you please confirm that also you have this issue?

Thanks!

One question why the aiida file is saved as a zip?

@superstar54 superstar54 self-requested a review October 10, 2024 08:28
@mikibonacci
Copy link
Member Author

Hi @AndresOrtegaGuerrero , it is just the standard archive format when you create an archive (see this in aiida-core create.py file at line 173.
Maybe I can just put the button description to be "Download AiiDA archive data" and not "Download AiiDA archive zip data", as it may be a bit misleading (the user may try to unzip the archive file).

What do you think?

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Hi @mikibonacci , thanks for the work!

One concern regarding large data. What if the download takes long time? Will it block the whole notebook?

I clicked the download button, and then went the step 1, I got this error:

DetachedInstanceError: Instance <DbNode at 0x7f05fd706280> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: https://sqlalche.me/e/20/bhk3)

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Hi @mikibonacci , thanks for the update. It's a good way to download it in another thread!

Maybe one bug: when download the raw data, it always in this status: Downloading data...

@mikibonacci
Copy link
Member Author

mikibonacci commented Oct 18, 2024

Hi @superstar54 , thanks for reviewing the PR. I addressed your valuable comments and suggestions.

However, I don't understand the bug you mention: are the data downloaded in your local machine but the "Downloading data..." still there?

Because if the data are a lot, it will take a while, and I think we should leave the string there up to completion

@superstar54
Copy link
Member

In my test, tt's a small calculation, the aidia file is downloaded within 5 s. But for the raw data, after 10 mins, it still shows Downloading data..., and no data is not downloaded.

This is needed for old AiiDA version which does not implement it.
@mikibonacci
Copy link
Member Author

Hi @superstar54 , I think I solved the issue. I just disable the raw data download if the ProcessDumper cannot be imported. I have not found a way to catch the error from the second thread (for now).

@superstar54 superstar54 self-requested a review October 18, 2024 14:58
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikibonacci mikibonacci merged commit c4a453b into aiidalab:main Oct 21, 2024
8 checks passed
@mikibonacci mikibonacci deleted the feature/download_data branch October 21, 2024 13:35
edan-bainglass pushed a commit to edan-bainglass/aiidalab-qe that referenced this pull request Oct 24, 2024
* Adding the download button for QeAppWorkChains.

we provide a DownloadDataWidget to download, separately:

- AiiDA archive: export_{node.pk}.aiida
- AiiDA raw data export_{node.pk}_raw.zip

both are zip files.

* Changing decode argument.

* Branch for debugging the non persistency

* improvements on the download widget:

- only one `_download_data` method, which allow to download raw and .aiida data (once at the time).
- QeAppWorkChain node is reloaded when the bitestream creation is done, this is needed to remain in the same Session (otherwise SQLAlchemy will complain that the Db request is not in the same session of the notebook.

* Improving docstring.

* Increasing time in test

* Reverting the selenium wait time in testing

* Download in a second thread, to not block the app.

* Added description to the download buttons

- moreover, flipped the button and the "Workflow completed successfully!" HTML.

* Doubling the waiting time in the test_qe_app_select_silicon_and_confirm

* Update src/aiidalab_qe/app/result/utils/download_data.py

Co-authored-by: Xing Wang <[email protected]>

* Update src/aiidalab_qe/app/result/utils/download_data.py

Co-authored-by: Xing Wang <[email protected]>

* Update src/aiidalab_qe/app/result/utils/download_data.py

Co-authored-by: Xing Wang <[email protected]>

* using Vertical box for buttons.

* Adding a message if the process dumper is not available.

This is needed for old AiiDA version which does not implement it.

* Adding the linter disable for the trial ProcessDumper import

* Adding init file in the new result/utils folder.

---------

Co-authored-by: Xing Wang <[email protected]>
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.

Add download buttons
3 participants