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

Remove submodules #23

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Remove submodules #23

wants to merge 8 commits into from

Conversation

jburel
Copy link
Member

@jburel jburel commented Aug 25, 2020

As discussed today
This PR removes submodules in favour of readthedocs sub-projects only.
This should fix the search issue described in #22
intersphinx is used to connect external repos. We cannot use the include but I think that's okay
The rest should work
Submodules have been removed.
cc @sbesson @manics @pwalczysko

This PR is for the approach: No submodule, readthedocs subproject

No submodule (git), readthedocs subproject (this PR)

pros:

cons:

submodule (git), readthedocs subproject (current implementation)

pros:

  • include is available
  • no url breakage
  • add ability to have the doc builds per PR

cons

  • search broken due to submodule
  • Edit on GitHub broken on some pages
  • submodule needs to be kept in synch (done via job opening PR)

submodule (git), No readthedocs subproject (alternative)

pros:

  • include is available
  • Fix the search at no cost
  • context menu (left-hand panel)

cons

  • url breakage: urls like https://omero-guides.readthedocs.io/projects/figure/en/latest/ will no longer be valid
  • Edit on GitHub broken on some pages
  • not possible to build docs per PR on sub-repo
  • submodule needs to be kept in synch (done via job opening PR)
  • require job to build the docs
  • long urls e.g. "figure/docs/"

One single omero-guides for doc (alternative)

pros:

  • search works
  • submodules don't need to be kept in sync
  • one build
  • technical API-related docs, closely tied to version number, should go to the corresponding repo

cons:

  • docs not alongside scripts. Manual copy of code snippet vs https://github.com/ome/omero-guide-python
  • how to handle when size grow
  • docs and binder. People have enjoyed having the info alongside the repo and be able to "run" the notebook and have textual content
  • from past experience, monolithic repo does not work and scale. With the number of guides growing, we need a better option

--exclude

@jburel jburel requested review from sbesson and manics August 25, 2020 17:42
@jburel jburel closed this Aug 25, 2020
@jburel jburel reopened this Aug 25, 2020
@sbesson
Copy link
Member

sbesson commented Aug 26, 2020

A few additions following this morning conversation:

  • contextual left-side menu: currently https://omero-guides.readthedocs.io/en/latest/iviewer/docs/ links to the whole hierarchy of guides defined by the submodule but https://omero-guides.readthedocs.io/projects/iviewer/ only shows the menu with the scope of the current guide. If going for the sub-project option, there might be the need to refer to one or several landing pages. This will obviously require a good balance as we don't want to reintroduce a tight coupling so that every guide must be knowledgeable about all the other others. One thing this repository does is not captured by the flat submodule is providing a hierarchy:

    Main page
            General concepts
                Introduction
                Upload
                 Script
           External Tools
               CellProfiler
               Fiji
               ....
    

    Maybe each low-level guide should link back to the intermediate landing page assuming we are happy with the general categories?

  • in terms of URLs, I find the sub-project one shorter and more usable. In particular, it strips the /docs bit which I believe is only an artifact of the source code layout. It also provides a more unified way of displaying the lang/version information at the end of URL i.e. <guide_domain>/<project>/<en,fr,de,...>/<latest>

  • breaking URLs would undoubtedly require some redirection work as we have linked to these resources extensively. We might want to tackle Renaming guides #5 at the same time if we decide to go that route to minimize the work on our side and the impact on the community.

@jburel
Copy link
Member Author

jburel commented Aug 31, 2020

Redirects can be put in place in readthedocs
Redirects will work when we have 404 (not before!)

@jburel
Copy link
Member Author

jburel commented Sep 9, 2020

The option to migrate guides into "code" repo e.g. omero-figure, omero-iviewer was discussed during last call.
I have looked at figure since it is the more complex due to the legacy (website, existing .rst file)
See https://github.com/jburel/omero-figure/tree/migrate_guide for output

@pwalczysko
Copy link
Member

The option to migrate guides into "code" repo e.g. omero-figure, omero-iviewer was discussed during last call.

Saw the figure repo and checked the rst. Although it seems as a viable option, cannot imagine a mainstream figure user to be able to find such doc, they need at least a link from the "main omero-guides docs", whatever that would be, otherwise the github repo would be not findable by them ?

@jburel
Copy link
Member Author

jburel commented Sep 9, 2020

@pwalczysko if we go for such option the subproject in omero-guides will be updated and point to omero-figure instead of omero-guide-figure. This initial work is only to see how the omero-figure repo ends up looking mainly due to the legacy.

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

From discussion:

  • options 1 or 3 are the ones that bring us where we want but none of them satisfy all our needs
  • @will-moore to look at combining some web apps (e.g. omero-guide-figure, omero-guide-iviewer) into one section web section in omero-guides. This should be done by filtering the repositories
  • option 4 was no longer considered. Some sections currently in separate repo might make it to the omero-guides repo (cf. point above)

To investigate:

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

I looked at the problem of the context menu on left-hand side.
It is possible to over the same menu on the subproject i.e. https://omero-guides.readthedocs.io/projects/iviewer/en/latest/ can offer the same menu as
https://omero-guides.readthedocs.io/en/latest/
The downside is that each subproject will need the same python script
See for example
https://mlbench.readthedocs.io/en/latest/ and https://mlbench.readthedocs.io/projects/mlbench_core/en/latest/

This means that if we add new entry in the toc in omero-guides, the change will have to be propagated to the other repositories.

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

Regarding the search:

Looking back at the example:

We could argue that it makes sense since it only searches within the context of the subproject when done at the subproject level and across all (parent+subproject) if the search is done at the parent level.

@manics
Copy link
Member

manics commented Sep 10, 2020

I think that's probably a good thing, especially as the number of guides increases and you're more likely to see matching results from multiple guides

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

I think it makes sense the way it is implemented since you refine your search context each time you go to a given component

@pwalczysko
Copy link
Member

We could argue that it makes sense since it only searches within the context of the subproject when done at the subproject level and across all (parent+subproject) if the search is done at the parent level.

Not sure if I understand it right: so if I am inside the ilastik subproject, I cannot successfully search for something which is in the fiji subproject ? If so, I do not like it, this is exactly the original search problem I was reporting about.

@manics
Copy link
Member

manics commented Sep 10, 2020

Is the search scope configurable in RTD?

@will-moore
Copy link
Member

I think it would have to be very clearly indicated that searching from a sub-project wouldn't find stuff higher up or from other projects.
Normally I wouldn't expect the current page to restrict what the pages I'm searching on. Better to get lots of search results than to not return a match that exists outside the search area.

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

@pwalczysko Reading the description of #22 the problem you described seems to be that you get "locked" at the subproject.
That should be fixed if we have the same menu at all levels

@pwalczysko
Copy link
Member

@pwalczysko Reading the description of #22 the problem you described seems to be that you get "locked" at the subproject.
That should be fixed if we have the same menu at all levels

Yes, same menu and same search results too. Maybe the issue is not stating it explicitly, but I can see two (equally important) ways of how to get out of being "locked"

  • link to top menu
  • search which encompasses the whole doc

They are equally important, maybe, it can be argued, the search even more than the menu link

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

https://docs.readthedocs.io/en/stable/subprojects.html

Search
Projects that are configured as subprojects will share a search index with their 
parent and sibling projects. This is currently the only way to share 
search indexes between projects, we do not yet support sharing search indexes 
between arbitrary projects.

so something might not be quite right in the project I was looking at or in our project

@jburel
Copy link
Member Author

jburel commented Sep 10, 2020

@will-moore
Copy link
Member

So, as I understood the discussion from last week, I was going to look at porting some of the non-code guides into the main omero-guides repo, to reduce the number of submodules? e.g:

@jburel
Copy link
Member Author

jburel commented Sep 16, 2020

Let's first look at the web apps as an example.

@will-moore
Copy link
Member

OK, so I'll start by porting iviewer guide into a top-level /iviewer/ directory in this repo?

@pwalczysko
Copy link
Member

OK, so I'll start by porting iviewer guide into a top-level /iviewer/ directory in this repo?

Whatever we do, we must remember that the guide is linked from www and docs and take care of the redirects or have some other strategy.

@jburel
Copy link
Member Author

jburel commented Sep 16, 2020

@pwalczysko for the redirect there is a mechanism in place in readthedocs
@will-moore Maybe webapps/iviewer. please use git branch filtering system when moving files from repo so we do keep the history. I know this is just an exploration but we do not want to have to do the work twice if we opt for that option

@will-moore
Copy link
Member

What does git branch filtering system mean?
Google suggests lots of things such as: https://git-scm.com/docs/git-filter-branch?

@will-moore
Copy link
Member

Do we really want the 'webapps' dir?
It seems like we get slightly cleaner URLs without it and I don't think we have so many web apps as to need it.
e.g. https://www.openmicroscopy.org/omero/iviewer/ and https://www.openmicroscopy.org/omero/figure/ are nicer than
https://www.openmicroscopy.org/omero/webapps/iviewer/ etc.

@jburel
Copy link
Member Author

jburel commented Sep 16, 2020

@jburel
Copy link
Member Author

jburel commented Sep 17, 2020

See #26

@sbesson
Copy link
Member

sbesson commented Sep 18, 2020

Reporting here an interesting issue related to the GitHub -> readthedocs workflow. Two successive PRs were merged in this repo bumping the submodules which triggered two builds: https://readthedocs.org/projects/omero-guides/builds/11901869/ and https://readthedocs.org/projects/omero-guides/builds/11901870/. For some reason, the first build completed after the second and overwrote the latest deployed version of omero-guides.readthedocs.io. A new build was re-triggered for the master branch to update the guides website.

Adding to this PR as this might be another point to take into consideration for the submodules decision. Note the problem is not fundamentally tied to submodules and the same type of race condition could happen when merging two PRs consecutively for any guide.

@manics
Copy link
Member

manics commented Sep 22, 2020

Have you opened an issue with read-the-docs about the race condition?

@sbesson
Copy link
Member

sbesson commented Sep 22, 2020

Have you opened an issue with read-the-docs about the race condition?

No but I have not investigated whether this is actually the expected behavior and/or whether an issue already existed which covers this workflow either

@jburel
Copy link
Member Author

jburel commented Feb 4, 2021

I went over the submodules vs subprojects option today with @pwalczysko since there was still some confusion about the context.
In order to have the navigation and search features requires when guiding our users, submodules is closer to what one will expect.
2 key points to compromise on:

  • The Edit on Github will be broken on some pages e.g. (Fiji). This could be fixed and it requires some investigation. Edit: It is possible to remove the button by overriding the templates see https://jburel-omeroweb-install.readthedocs.io/en/latest/
  • usage of the rtd hook on the subrepo will not be available. Mechanism to review PR will need to be put in place.

@sbesson sbesson mentioned this pull request Jul 25, 2022
@sbesson sbesson removed their request for review September 7, 2022 19:15
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.

5 participants