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

Patch search index #4292

Merged
merged 4 commits into from
Nov 13, 2024
Merged

Patch search index #4292

merged 4 commits into from
Nov 13, 2024

Conversation

aaruni96
Copy link
Member

@aaruni96 aaruni96 commented Nov 7, 2024

This removes anything in search index that is not also present in doc.main. That is, the search function will not return "hidden" pages, which are not available via normal navigation. This was easier than trying to make Documenter make a nice search index to begin with which needed more digging into ( JuliaDocs/Documenter.jl#2517 )

Address @lgoettgens comment from #3816 :

Can you maybe just disable all results from files that are not contained in the doc.main file, i.e. the list of files reachable via the menu?

Replaces #4290

This removes anything in search index that is not also present in doc.main.
That is, the search function will not return "hidden" pages, which are not
available via normal navigation.
@aaruni96 aaruni96 mentioned this pull request Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.47%. Comparing base (24711ee) to head (d4e5ad8).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4292      +/-   ##
==========================================
- Coverage   84.49%   84.47%   -0.02%     
==========================================
  Files         641      641              
  Lines       85476    85541      +65     
==========================================
+ Hits        72226    72264      +38     
- Misses      13250    13277      +27     

see 14 files with indirect coverage changes

@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

Docs deployment failed because I load JSON, and JSON is not available. In principle I understand this, but I am now confused about how Julia works. I thought Julia only makes the packages added to the current project available for import ?

Here's my Project.toml:

$ cat Project.toml 
[deps]
Oscar = "f1435218-dba5-11e9-1e4d-f1a5fab5fc13"
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe"

My invocation of julia and using Json:

$ julia --project=./
The latest version of Julia in the `release` channel is 1.11.1+0.x64.linux.gnu. You currently have `1.10.4+0.x64.linux.gnu` installed. Run:

  juliaup update

in your terminal shell to install Julia 1.11.1+0.x64.linux.gnu and update the `release` channel to that version.
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.4 (2024-06-04)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using JSON

help?> JSON.parse
  parse(str::AbstractString;
        dicttype::Type{T}=Dict,
        inttype::Type{<:Real}=Int64,
        allownan::Bool=true,
        null=nothing) where {T<:AbstractDict}
...

So, on my local computer, using JSON works without ] add JSON, but on the CI it doesn't ?

Coming back to point, since JSON is apparently an external dependency, do we want to add it to Oscar ? Are there other workarounds to parse json files into a vector of dicts ?

@benlorenz
Copy link
Member

You might have JSON in your global environment (the projects are layered, you can also load stuff from there even with --project=.).

But Oscar already depends on JSON, so you should be able to just use Oscar.JSON.parse.

@lgoettgens
Copy link
Member

Alternatively, you can add JSON to docs/Project.toml

@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

Is there a preferred answer between these two alternatives ?

@benlorenz
Copy link
Member

I would say adding it explicitly to the docs project is cleaner and doesn't hurt.

@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

Looks like all works, and documentation preview is available at https://docs.oscar-system.org/previews/PR4292/

@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

Again, I don't get it. The search index created by CI is emtpy ( https://docs.oscar-system.org/previews/PR4292/search_index.js ). The search index created on my host has a lot of entries in it.

$ head search_index.js -c 100 
var documenterSearchIndex = {"docs":
[{"location":"Groups/subgroups.html","page":"Subgroups","title"

$ ls -lh search_index.js 
-rw-r--r-- 1 aaruni aaruni 3.5M Nov  8 11:36 search_index.js

I don't have any local changes which I haven't committed.

$ git status
On branch ak96/patch-search-index
Your branch is up to date with 'upstream/ak96/patch-search-index'.

nothing to commit, working tree clean

@benlorenz
Copy link
Member

I do get an (almost) empty file when running this locally:

$ cat docs/build/search_index.js
var documenterSearchIndex = {"docs":
[]
}

One thing that might be an issue is that the filelist has .html at the end for each entry but the locations in the search index don't have those (but a slash instead):

  {
    "location": "NoncommutativeAlgebra/PBWAlgebras/ideals/#is_zero-Tuple{Oscar.PBWAlgIdeal}",
    "page": "Ideals in PBW-algebras",
    "title": "is_zero",
    "text": "is_zero(I::PBWAlgIdeal)\n\nReturn true if I is the zero ideal, false otherwise.\n\nExamples\n\njulia> D, (x, y, dx, dy) = weyl_algebra(QQ, [:x, :y])\n(Weyl-algebra over Rational field in variables (x, y), PBWAlgElem{QQFieldElem, Singular.n_Q}[x, y, dx, dy])\n\njulia> I = left_ideal(D, [x, dx])\nleft_ideal(x, dx)\n\njulia> is_zero(I)\nfalse\n\n\n\n\n\n",
    "category": "method"
  },

@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

I test using Oscar.build_doc(; doctest=false, warnonly=true, open_browser=false, start_server=false)

Does this do something different which adds .html at the end ? The same entry in my search index is

{
  "location": "NoncommutativeAlgebra/PBWAlgebras/ideals.html#is_zero-Tuple{Oscar.PBWAlgIdeal}",
  "page": "Ideals in PBW-algebras",
  "title": "is_zero",
  "text": "is_zero(I::PBWAlgIdeal)\n\nReturn true if I is the zero ideal, false otherwise.\n\nExamples\n\njulia> D, (x, y, dx, dy) = weyl_algebra(QQ, [:x, :y])\n(Weyl-algebra over Rational field in variables (x, y), PBWAlgElem{QQFieldElem, Singular.n_Q}[x, y, dx, dy])\n\njulia> I = left_ideal(D, [x, dx])\nleft_ideal(x, dx)\n\njulia> is_zero(I)\nfalse\n\n\n\n\n\n",
  "category": "method"
}

@benlorenz
Copy link
Member

benlorenz commented Nov 8, 2024

I ran the commands from the CI yml, i.e. julia --project=. --color=yes -e 'using Oscar; Oscar.doc_init(; path="docs/") and julia --project=docs/ --color=yes docs/make.jl.

Edit: make.jl has local_build=false which translates to prettyurls=true for documenter which might affect these locations.

docs/make_work.jl Outdated Show resolved Hide resolved
@aaruni96
Copy link
Member Author

aaruni96 commented Nov 8, 2024

I ran the commands from the CI yml

This does not work for me at all, i.e., the search_index.js isn't created and the program falls down when trying to read from it (because it assumes it will always exist). I tried putting the entire patch block inside an if Base.Filesystem.ispath(joinpath(docspath, "build", "search_index.js")), and then code completes, but search_index.js is really not created at all

$ find docs/ -name "*.js"
docs/build/AbstractAlgebra/mathjaxhelper.js
docs/build/Nemo/mathjaxhelper.js
docs/build/assets/themeswap.js
docs/build/assets/warner.js
docs/build/assets/documenter.js

The exact command I ran was

julia --project=. --color=yes -e 'using Oscar; Oscar.doc_init(; path="docs/")' && julia --project=docs/ --color=yes docs/make.jl

@benlorenz
Copy link
Member

I pushed a commit using julia for parsing the doc.main which also uses either .html or / as suffix. This did work locally both when running the CI commands and when using build_doc. CI is still running...

@benlorenz
Copy link
Member

Looks like it worked, the search index is not empty anymore, search seems to work and the file is 3.4MB instead of the 5.4MB on the current master.

@fingolfin
Copy link
Member

Preview for testing is at https://oscar-system.github.io/Oscar.jl/previews/PR4292/

@fieker
Copy link
Contributor

fieker commented Nov 13, 2024

@HereAround will briefly approve it

@HereAround
Copy link
Member

HereAround commented Nov 13, 2024

@aaruni96 I just checked the preview at https://docs.oscar-system.org/previews/PR4292/manualindex/. It seems that a number of search entries appear many times. For instance, agm is listed 6 times. Likewise, ambient_dim is listed 8 times. There are many, many more such examples.

I believe this is because OSCAR uses the same function name with a (possibly large) number of distinct signatures. Would it make sense to add those signatures to the index? As I see it, this should enhance the usefulness of the index significantly. We can also postpone this change, but let us open an issue in this case, so we will not forget. Still - as long as I am concerned - this PR can be merged.

@aaruni96 What are your thoughts?

@aaruni96
Copy link
Member Author

I think this may be a feature, not a bug. Keeping in mind the example of ambient_dim, the search has no way of knowing which ambient_dim you are looking for, and all 8 of them mean different things (which you can see by the different URLs they present). Except if you want to document ambient_dim as a separate unifying page which you want the search result to return, I don't see how this can be changed (or even whether this should be changed?).

@HereAround
Copy link
Member

I think this may be a feature, not a bug. Keeping in mind the example of ambient_dim, the search has no way of knowing which ambient_dim you are looking for, and all 8 of them mean different things (which you can see by the different URLs they present). Except if you want to document ambient_dim as a separate unifying page which you want the search result to return, I don't see how this can be changed (or even whether this should be changed?).

@aaruni96 and I just briefly exchanged on slack. Bottom-line: Improvements are always possible... at some point in the future (e.g. after the upcoming MaRDI workshops).

Thank you for working on this @aaruni96 !

@HereAround HereAround merged commit d15b0c9 into master Nov 13, 2024
30 checks passed
@HereAround HereAround deleted the ak96/patch-search-index branch November 13, 2024 13:03
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