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

Mig/2.1 #271

Merged
merged 18 commits into from
Nov 22, 2022
Merged

Mig/2.1 #271

merged 18 commits into from
Nov 22, 2022

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Aug 29, 2022

fixes #272

@unkcpz
Copy link
Member Author

unkcpz commented Oct 10, 2022

Since it is able to install the qe code and sssp library by post-install, and the aiidalab full-stack have that feature build-in.
We do not need to install the qe code the first time App start.
Therefore, I simply using auto_start=False to disable the auto_start of the code setup which will get rid of the thread issue.
But the thread issue should anyway be solved and the code setup widget need to be decoupled and refactored to the AWB. I open a dedicate issue for it #299.

@unkcpz unkcpz marked this pull request as ready for review October 10, 2022 09:33
@superstar54 superstar54 self-requested a review November 14, 2022 10:59
@unkcpz unkcpz changed the title Mig/2.x Mig/2.1 Nov 14, 2022
pyproject.toml Outdated Show resolved Hide resolved
src/setup.cfg Outdated Show resolved Hide resolved
@danielhollas
Copy link
Contributor

@unkcpz FYI by turning on the deprecation warnings by setting AIIDA_WARN_v3=1 env variable, I identified a few more things in my app that should be upgraded, such as deprecated set_extra and get_extra methods and some others. They are also relevant here, you might want to take a look at this commit ispg-group/aiidalab-ispg@26157c1

btw: AiiDA v2.1.2 has been released yesterday.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 17, 2022

@danielhollas thanks for the advice. But how to turn on the config exactly, are the warning messages will show on the notebook? But the shell environment variables only have an effect on the shell that trigger it, correct?

@danielhollas
Copy link
Contributor

danielhollas commented Nov 17, 2022

Yes, it is a bit tricky. Since I am building my own Docker image, I have simply set the environment variable in the Dockerfile.

And yes, the warnings show up in the notebook UI. They might get swallowed in the Appmode, I don't remember.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 17, 2022

Seems it can be set for the notebook by ipython magic https://stackoverflow.com/questions/37890898/how-to-set-env-variable-in-jupyter-notebook. I'll give it a try tomorrow.

I wonder if maybe there is AiiDA API to trigger this (AIIDA_WARN_v3=1)? @sphuber

@sphuber
Copy link

sphuber commented Nov 17, 2022

I wonder if maybe there is AiiDA API to trigger this (AIIDA_WARN_v3=1)? @sphuber

It is just an environment variable, so I guess you could do

import os
os.env['AIIDA_WARN_v3'] = True

@unkcpz
Copy link
Member Author

unkcpz commented Nov 21, 2022

Hi @superstar54, after the release of aiida-quantumespresso, I think there is no block for testing with this branch, can you test and review it again? Thanks!

qe.ipynb Outdated
"metadata": {
"scrolled": false
},
"outputs": [],
"outputs": [
Copy link
Member Author

Choose a reason for hiding this comment

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

Clean up this nb file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, thanks I'll give it a try.

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.

I tested this PR by running a few examples with Si (diamond) structure.

  • scf only
  • scf + bands + pdos
  • relax
  • relax + bands + pdos

All the tests passed. Great!

There are two points which could be improved in the feature.

  • After the calculation, I need to refresh the page to load the results (structure and bands).
  • the pdos above the fermi level shift a little bit away from the axis, as shown below:

@unkcpz
Copy link
Member Author

unkcpz commented Nov 22, 2022

the pdos above the fermi level shift a little bit away from the axis, as shown below:

We have updated the widget-bandsplot, but in another PR. @dou-du and I make a lot changes of to the widget, so maybe the problem solved naturally. We can test it there.

@superstar54
Copy link
Member

@unkcpz I think we can merge this PR after you clean up the nb file.

The test only covers step 1 (structure selection). However, since I did all tests manually, I think it should be fine. In the future, tests on steps 2~4 should also be added.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 22, 2022

The test only covers step 1 (structure selection). However, since I did all tests manually, I think it should be fine. In the future, tests on steps 2~4 should also be added.

Totally agree!

@dou-du
Copy link

dou-du commented Nov 22, 2022

I tested this PR by running a few examples with Si (diamond) structure.

* scf only

* scf + bands + pdos

* relax

* relax + bands + pdos

All the tests passed. Great!

There are two points which could be improved in the feature.

* After the calculation, I need to refresh the page to load the results (structure and bands).

* the pdos above the fermi level shift a little bit away from the axis, as shown below:

This bug has been fixed in our latest released version.

@unkcpz unkcpz force-pushed the mig/2.x branch 5 times, most recently from 7b2b3ed to e1530d7 Compare November 22, 2022 23:19
@unkcpz
Copy link
Member Author

unkcpz commented Nov 22, 2022

The test only covers step 1 (structure selection). However, since I did all tests manually, I think it should be fine. In the future, tests on steps 2~4 should also be added.

I open the issue in #316

The failed test is fixed. Somehow the firefox test failed because firefox is not installed properly. Since @superstar54 already mentioned the PR is ready. I merge it without approval.

Add - uses: browser-actions/setup-geckodriver@latest

test
- uses: browser-actions/setup-firefox@latest
with:
firefox-version: ${{ matrix.firefox }}
run: which firefox
Copy link
Contributor

Choose a reason for hiding this comment

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

@unkcpz I think this line broke this whole file and tests are not running.

https://github.com/aiidalab/aiidalab-qe/actions/runs/3527890535

obrazek

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I simply see All test pass. My fault, will open a PR to fix it.

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.

QE app: Support AiiDA core 2.0
5 participants