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

add improved search using rtd-sphinx-search #4111

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented May 25, 2020

@ltalirz ltalirz requested a review from csadorf May 25, 2020 12:34
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

I think it looks great, I'm just a bit worried about the dependency.

docs/requirements_for_rtd.txt Outdated Show resolved Hide resolved
@sphuber sphuber changed the base branch from docs-revamp to develop June 2, 2020 18:15
@sphuber
Copy link
Contributor

sphuber commented Jun 2, 2020

I rebased the branch and updated the base branch to be develop now that docs-revamp has been merged. We can see how it works and looks in the new docs.

@sphuber
Copy link
Contributor

sphuber commented Jun 2, 2020

The real thing that is still blocking the search from being useful is still that the results aren't particularly useful. The API reference dominates the search results, and I think this is rarely what a user is after. I have been googling for quite a while now to see if we can configure or customize the search behavior and maybe penalize the API reference, but so far no dice.

@ltalirz
Copy link
Member Author

ltalirz commented Jun 2, 2020

The real thing that is still blocking the search from being useful is still that the results aren't particularly useful. The API reference dominates the search results, and I think this is rarely what a user is after.

Yeah, something like this: readthedocs/readthedocs.org#7082
Maybe give it a thumbs up, people should know this is important

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

install_requirements.update({Requirement.parse(r) for r in setup_cfg['extras_require'][key]})

# add requirement for "search as you type"
install_requirements.update({Requirement.parse('readthedocs-sphinx-search~=0.1.0')})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add this to the docs extra? I know it only makes sense on RTD, but can't hurt to install it, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well... I would say things I don't need hurt my disk ;-)
I'm not strongly against including it, if others think this is better

docs/requirements_for_rtd.txt Outdated Show resolved Hide resolved
utils/dependency_management.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #4111 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4111      +/-   ##
===========================================
+ Coverage    78.89%   78.89%   +0.01%     
===========================================
  Files          467      467              
  Lines        34481    34481              
===========================================
+ Hits         27199    27202       +3     
+ Misses        7282     7279       -3     
Flag Coverage Δ
#django 70.82% <ø> (ø)
#sqlalchemy 71.70% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
aiida/transports/plugins/local.py 80.47% <0.00%> (+0.26%) ⬆️
aiida/engine/daemon/client.py 73.72% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c817021...abd7004. Read the comment docs.

@ltalirz
Copy link
Member Author

ltalirz commented Jun 10, 2020

@csadorf thanks for the note, this came from the rebase (fixed now).
The docs test is passing now, but some django tests are randomly failing with

Fatal Python error: Illegal instruction

Is this a known issue?

@sphuber
Copy link
Contributor

sphuber commented Jun 10, 2020

Is this a known issue?

Haven't ever seen that one before, but it seems that an illegal instruction is executed on a different thread, which almost certainly that of the RMQ communicator. Not sure why this would pop up now. We have been refactoring kiwipy and plumpy to move to asyncio, but none of this has been released yet and the pip freeze confirms that the old (the latest released ones) versions are installed. Think this might just be a question of rerunning the jobs and seeing if it disappears by itself

@ltalirz
Copy link
Member Author

ltalirz commented Jun 10, 2020

Ok. For the record, this is an example of a failing CI run:
https://github.com/aiidateam/aiida-core/pull/4111/checks?check_run_id=757657634

And the error message is:
(seems to be some combination of pymatgen and plumpy)

tests/test_base_dataclasses.py .................                         [  1%]
tests/test_calculation_node.py .....                                     [  1%]
Fatal Python error: Illegal instruction

Thread 0x00007fefcddbe700 (most recent call first):
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/ioloop.py", line 863 in start
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/kiwipy/rmq/communicator.py", line 543 in run_loop
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/threading.py", line 870 in run
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/threading.py", line 890 in _bootstrap

Current thread 0x00007feff77af740 (most recent call first):
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/lattice.py", line 1108 in get_points_in_sphere
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/lattice.py", line 610 in find_all_mappings
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/lattice.py", line 687 in find_mapping
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/lattice.py", line 932 in get_niggli_reduced_lattice
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/structure.py", line 1602 in get_reduced_structure
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/core/structure.py", line 1984 in get_primitive_structure
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/io/cif.py", line 1087 in _get_structure
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pymatgen/io/cif.py", line 1108 in get_structures
  File "/home/runner/work/aiida-core/aiida-core/aiida/tools/data/cif.py", line 133 in _get_aiida_structure_pymatgen_inline
  File "/home/runner/work/aiida-core/aiida-core/aiida/engine/processes/functions.py", line 409 in run
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/process_states.py", line 220 in execute
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/types.py", line 278 in wrapped
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 292 in wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/stack_context.py", line 390 in run_with_stack_context
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 503 in _run_task
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 307 in wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 1084 in step
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 307 in wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 88 in func_wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 1116 in step_until_terminated
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 1069 in run
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 1003 in __init__
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/gen.py", line 319 in wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/ioloop.py", line 436 in run
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/stack_context.py", line 277 in null_wrapper
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/ioloop.py", line 605 in _run_callback
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/ioloop.py", line 832 in start
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/tornado/ioloop.py", line 453 in run_sync
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 1068 in execute
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/plumpy/processes.py", line 88 in func_wrapper
  File "/home/runner/work/aiida-core/aiida-core/aiida/engine/processes/functions.py", line 361 in execute
  File "/home/runner/work/aiida-core/aiida-core/aiida/engine/processes/functions.py", line 153 in run_get_node
File "/home/runner/work/aiida-core/aiida-core/aiida/engine/processes/functions.py", line 181 in decorated_function
  File "/home/runner/work/aiida-core/aiida-core/aiida/orm/nodes/data/cif.py", line 770 in get_structure
  File "/home/runner/work/aiida-core/aiida-core/tests/test_dataclasses.py", line 318 in test_ase_primitive_and_conventional_cells_pymatgen

...

  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/opt/hostedtoolcache/Python/3.8.3/x64/lib/python3.8/site-packages/_pytest/config/__init__.py", line 124 in main
  File "/opt/hostedtoolcache/Python/3.8.3/x64/bin/pytest", line 8 in <module>
.github/workflows/tests.sh: line 35: 13645 Illegal instruction     (core dumped) AIIDA_TEST_PROFILE=test_$AIIDA_TEST_BACKEND pytest tests
tests/test_dataclasses.py .

Indeed this is using a rather recent release of pymatgen:
Downloading pymatgen-2020.6.8-cp38-cp38-manylinux2010_x86_64.whl (4.4 MB)

@greschd
Copy link
Member

greschd commented Jun 10, 2020

I think "Illegal instruction" means it's trying to execute a CPU instruction that is not supported.

My guess would be the pymatgen wheel was built against a CPU architecture that is not (fully) supported by the gitlab CI VMs (either because the host doesn't support it, or because it's not exposed to the VM).

Here's a related pymatgen issue: materialsproject/pymatgen#1868

And an old issue from numpy: numpy/numpy#9532
Numpy seems to have solved this by checking /proc/cpuinfo at run-time.

@greschd
Copy link
Member

greschd commented Jun 10, 2020

Update on the failing test: should be fixed, pymatgen removed the offending wheels.

@ltalirz
Copy link
Member Author

ltalirz commented Jun 10, 2020

Thanks @greschd ! Problems that solve themselves are the nicest problems :-)

Ready for re-review!

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@csadorf csadorf requested a review from sphuber June 11, 2020 07:52
@ltalirz
Copy link
Member Author

ltalirz commented Jun 11, 2020

Ok, then let's give this a try - let's see how people react ;-)

@ltalirz ltalirz merged commit 5f486e3 into develop Jun 11, 2020
@ltalirz ltalirz deleted the rtd-elasticsearch branch June 11, 2020 08:15
@chrisjsewell
Copy link
Member

chrisjsewell commented Jun 17, 2020

@ltalirz ermm my reaction is that its faulty lol; the search box appears behind the builtin one, so you can't actually see what you are typing:

image

(perhaps some custom CSS can sort this out)

@sphuber
Copy link
Contributor

sphuber commented Jun 17, 2020

For me it works. Maybe it is due to your dark theme CSS? Can you try the default one?

@chrisjsewell
Copy link
Member

chrisjsewell commented Jun 17, 2020

No that doesn't change anything. What browser are you using? above was on Firefox. On chrome it is still underneath, although at least I can see the search bar:

image

@chrisjsewell
Copy link
Member

Also of note is that it fixes itself when the screen gets below a certain width (i.e. via the CSS @media rules)

@greschd
Copy link
Member

greschd commented Jun 17, 2020

Jup, can confirm having the same issue - tested in Firefox and Edge. Works in Internet Explorer though 🤣

@bosonie
Copy link

bosonie commented Jun 17, 2020

Also, in my Firefox, when I type enter, I then can't correct (modify) the word I'm searching. I'm forced to write it back fully. Also, I can't paste there anything.
Basically, if I misspell a word, I'm screwed 🤣

@greschd
Copy link
Member

greschd commented Jun 17, 2020

Also, in my Firefox, when I type enter, I then can't correct (modify) the word I'm searching. I'm forced to write it back fully. Also, I can't paste there anything.
Basically, if I misspell a word, I'm screwed 🤣

That actually seems like a more serious UX issue - I'd vote to revert for now, until we can fix some of these issues.

@sphuber
Copy link
Contributor

sphuber commented Jun 17, 2020

I'm on firefox on Ubuntu 18.04 and seems to work fine on a 4k screen so width as a cause seems unlikely

@chrisjsewell
Copy link
Member

so width as a cause seems unlikely

Jun-17-2020 10-18-58

anyway I have opened a issue: pydata/pydata-sphinx-theme#202, so I would vote to revert for now and monitor the progress of that issue

@greschd
Copy link
Member

greschd commented Jun 17, 2020

Should we also open issues for the paste / edit problems on rtd-sphinx-search?

@bosonie
Copy link

bosonie commented Jun 17, 2020

If someone else experiences the same problem, yes. Do you @greschd ?

@sphuber
Copy link
Contributor

sphuber commented Jun 17, 2020

so width as a cause seems unlikely

What I meant was that for me it does not seem to depend on the width, using the same browser, so it cannot be exclusively a width problem

@greschd
Copy link
Member

greschd commented Jun 17, 2020

@bosonie I can paste, but when I try to edit a previous search the search string is in the "wrong" search bar.

@sphuber
Copy link
Contributor

sphuber commented Jun 17, 2020

Regarding the other issues, yeah the UI is quite flaky. If you search and it finds some results, you can no longer edit it. There is the cross mark in the top right, which you might think resets the search bar, but in fact it just closes the window

@bosonie
Copy link

bosonie commented Jun 17, 2020

Ok, I review the "paste" problem and probably I had a problem with my keyboard. Now Ctrl+V works, but still I can't paste with the central button of the mouse, a feature that was present in the old system. Moreover, you can't copy from the search bar below, therefore you can't copy your old searched word.
Regardless, the impossibility of editing the search bar after a search is something we should report.

@ltalirz
Copy link
Member Author

ltalirz commented Jun 17, 2020

If this produces more issues than it solves, I'm perfectly fine with reverting this.
Can someone take care of preparing the PR?

@ltalirz
Copy link
Member Author

ltalirz commented Jun 17, 2020

Hm... I will prepare it myself - I think some of the changes in the validation script should remain

@ltalirz
Copy link
Member Author

ltalirz commented Jun 17, 2020

See #4180

@chrisjsewell
Copy link
Member

thanks @ltalirz, I really like the UX (when working), so hopefully we can get it fixed soon and add it back in 🤞

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.

6 participants