♻️ REFACTOR: Remove reentry requirement
#5058
Conversation
|
http://www.quantum-espresso.org/ is currently down with crazy error 😬 (hence linkcheck failing) |
|
Just to understand: are you building on top of #4960 or not? Have all references to reentry been removed from the codebase? |
no
yes |
Codecov Report
@@ Coverage Diff @@
## develop #5058 +/- ##
===========================================
+ Coverage 80.39% 80.39% +0.01%
===========================================
Files 529 529
Lines 36881 36882 +1
===========================================
+ Hits 29646 29649 +3
+ Misses 7235 7233 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I notice that you keep updating the PR.
I'll stop going through it now and come back later - please let me know once it's ready for review.
As for using the importlib_metadata: I haven't yet looked at your implementation, but I would just like to mention that in python 3.9 (that has the importlib.metadata in the standard library) using the importlib_metadata package as a drop-in replacement was significantly slower, see #3458 (comment)
I.e. if we are going to rely on this, we should make sure to use the fast implementation whenever it is available (maybe you already do this; I will check later).
| @@ -1,5 +1,5 @@ | |||
| [build-system] | |||
| requires = ["setuptools>=40.8.0", "wheel", "reentry~=1.3", "fastentrypoints~=0.12"] | |||
There was a problem hiding this comment.
please remember to mention in the eventual commit message that fastentrypoints is also being removed
There was a problem hiding this comment.
just to check, is there a reason you know of why we would still need this? I believe the issue it solved is no longer an issue?
There was a problem hiding this comment.
Can you compare the cli scripts produced by pip-installing the PR (e.g. the verdi one) against the current ones?
If the new ones do not import pkg_resources, I guess we can remove the dependency.
There was a problem hiding this comment.
This is without fastentrypoints, so I guess it is now ok to remove:
#!/Users/chrisjsewell/Documents/GitHub/aiida_core_develop/.tox/py38-pre-commit/bin/python
# EASY-INSTALL-ENTRY-SCRIPT: 'aiida-core','console_scripts','verdi'
import re
import sys
# for compatibility with easy_install; see #2198
__requires__ = 'aiida-core'
try:
from importlib.metadata import distribution
except ImportError:
try:
from importlib_metadata import distribution
except ImportError:
from pkg_resources import load_entry_point
def importlib_load_entry_point(spec, group, name):
dist_name, _, _ = spec.partition('==')
matches = (
entry_point
for entry_point in distribution(dist_name).entry_points
if entry_point.group == group and entry_point.name == name
)
return next(matches).load()
globals().setdefault('load_entry_point', importlib_load_entry_point)
if __name__ == '__main__':
sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
sys.exit(load_entry_point('aiida-core', 'console_scripts', 'verdi')())
There was a problem hiding this comment.
Yeh so actually the current develop verdi script is:
#!/Users/chrisjsewell/Documents/GitHub/aiida-core-origin/.tox/py39-pre-commit/bin/python
# -*- coding: utf-8 -*-
# EASY-INSTALL-ENTRY-SCRIPT: 'aiida-core','console_scripts','verdi'
__requires__ = 'aiida-core'
import re
import sys
from aiida.cmdline.commands.cmd_verdi import verdi
if __name__ == '__main__':
sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
sys.exit(verdi())
I guess you need importlib_metadata installed to produce the new script?
There was a problem hiding this comment.
So I re-added fastentrypoints: I guess that the second script, with the direct verdi import, is still sightly quicker
yeh sorry just minor fixes to the tests |
the complication here though comes in that |
|
I would also note that from v4.3.0, there is a claimed performance improvement: python/importlib_metadata#317 |
|
Just playing around with autocompletion on my computer, and the performance seems good |
You upped the time limit for the load time of verdi to 0.5. Is that still necessary after the update to a more recent |
Haha you noticed my cheat, yeh a few of them were sneaking over (I added a run for all python versions)
Comparing that to the latest run on develop (python 3.8): SUCCESS: loading time 0.34 at iteration 1 below so not much in it |
reverted |
|
ok @sphuber, do you want to give this another try before I merge today? |
|
@chrisjsewell, just to clarify if I can look into this in the next few days: do we have until the next dev meeting or you are going to merge this today? |
I would say that, now we have made this final improvement (6689fd2) and removed any significant timing difference to reentry, there is no need to wait. Unless obviously you feel strongly we should? |
|
From my side I agree that there is no reason to wait anymore |
|
@ltalirz, if you have time, perhaps you could quickly pop a new comparison of the new vs reentry times on here? (I'm working on another branch at the mo, so can't) |
|
this branch
This is really negligible and won't be noticed by users (we're also getting into a region where I would need to make sure the difference is really reproducible) |
|
Thanks for the clarification, it was just since you asked for feedback and I didn't understand that the time difference has disappeared. If the time difference is negligible I'm ok with the merge. This branch: develop: So I'm OK to merge. However, while looking into it with cProfile+snakeviz, I realised that a call to cmd_computer was taking a lot of time (~80ms) even when the command was not involved (e.g. when expanding "verdi data bands sho"). In this branch, if you comment out the whole content of the from aiida.transports import cli as transport_cliThen the time goes up again. If you want to give a look into this already now, great, otherwise I'm ok with merging this as is since this would be a further improvement, but in this case could you please open an issue mentioning this? (I also have the feeling that there might be at least one or two similar things one could improve, e.g. also cmd_archive seems to be taking some time, but the cProfile+snakeviz is quite complex to "parse" and I still don't know if it's in addition to the time of cmd_computer or not). |
|
thanks @giovannipizzi! I will indeed have a quick check |
FYI @giovannipizzi maybe check out https://github.com/benfred/py-spy, which is a lot easier to use |
|
commit message: This commit replaces the use of aiida-core makes heavy use of entry-points to define plugins. In recent years For now, we use |
|
ah @sphuber you ** lol, merging commits when I'm almost finished running the branch update |
|
are you gonna be merging anymore? |
Can't help it you're not quick enough, I updated the other branch before you updated this one. But yeah, not merging anything in the near future. |
Utilise the python 3.10
selectAPI (https://docs.python.org/3.10/library/importlib.metadata.html), via importlib-metadata.The code currently includes a fall back to the old API, since I have previously encountered an issue with old versions of importlib_metadata being installed on Conda: python/importlib_metadata#308Also added typing to module and mypy type checking