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

📚 Multiple docs improvements #1745

Closed
wants to merge 3 commits into from
Closed

Conversation

daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Jun 7, 2024

Sorts out:

  • Faster docs build
  • Full API representation Going through notebooks and adding common methods instead into custom_autosummary
  • Reduces number of sphinx warnings and docs issues
    • More consistent title levels
    • Adding missing documentation onto stub files
  • Adds the latest lectures on the webcenter
  • Ugly attr directory inclusion on each class in the pre/2.7 docs

I have been trying multiple approaches to reduce the duration of the docs build, but as the project increases, we might have to consider a few alternatives.

Things I have tried:

  • include_patterns and exclude_patterns so sphinx only recognizes the documents related to the documentation build
  • [-] Trying to reduce sphinx warnings
  • We could remove the source code linking but would be nice not to have to do that (it seems to be increasing the sitemap size so catching SEO?)
  • We could commit the autosummary and generate it as we prefer, but this is not an ideal option as it won't be automatically updated on existing generated files. It would speed up the build.
  • We might have to request rtd to increase our documentation build time if none else works

General Development FLow

Related PRs to be merged before this one:

@daquinteroflex
Copy link
Collaborator Author

Note that the docs sync issue is fixed in pre/2.8 accordingly here. I will manually push to that remote, but I believe it will be fine to merge to develop for now in any case.

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Jul 26, 2024

So docs should be finished building anytime live here https://docs.flexcompute.com/projects/tidy3d/en/docs-demo-full_api/

Note that our docs builds after this are >2hrs on RTD

@tylerflex
Copy link
Collaborator

Thanks @daquinteroflex , so this PR adds back listing of all fields in the API reference? and 2 hours is better than not building at all!

@tylerflex
Copy link
Collaborator

I definitely like it better with all of the methods. A few comments:

  • Would be nice if somehow possible to put the tidy3d-defined ones first. (or at least the ones defined closest to the class in the inheritance hierarchy). If not possible it's probably fine. There's a good argument for alphabetical, especially with many methods.
  • I think we could exclude the dunder methods, they don't really add much and show up first. Is it possible to remove?

Implementation wise, looks good so I'm approving it for that. Also really appreciate all the nice additions!

@@ -64,8 +64,10 @@
autodoc_class_signature = "separated"
autodoc_default_options = {
"members": True,
"inherited-members": True,
"member-order": "bysource",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the existing configuration, there are a few options. I believe the methods can be ordered by source or alphabetical. Unfortunately, we don't have more control about this in my understanding than this parameter. I had intended to give it some time iterating a few approaches, but then realised we might already be limited on this.

I've raised an issue for the new docs extension to enable this flexcompute/autoflex#12

However, maybe for now this is it?

Copy link
Collaborator

@tylerflex tylerflex Jul 31, 2024

Choose a reason for hiding this comment

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

one or the other (source or alphabetical) is probably fine enough for now.

docs/conf.py Outdated
"member-order": "bysource",
"undoc-members": True,
"exclude-members": "SchemaConfig,__init__,Config,attrs,chunk,copy,json,log",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to try a few more approaches to remove the dunder methods too so that the inherited members are not recognised.

Am afraid am unable to do these changes until later this afternoon as have been caught up this start of week.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea no rush! thanks

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Jul 31, 2024

Unfortunate news, it seems like the latest build has again failed despite two previous successful builds. Emailing read-the-docs again.

They did mention this build does take a lot of concurrency time, and I did notice another branch build occured at the same time which meant that it could have taken resources out. I'm trying rebuilding now to see if that works this time. However, this would have important implications of reducing the frequency of our documentation builds - possibly only in the latest branch or when triggered manually?

@tylerflex
Copy link
Collaborator

However, this would have important implications of reducing the frequency of our documentation builds - possibly only in the latest branch or when triggered manually?

That's probably fine from my perspective. So right now when do they get rebuilt automatically?

@daquinteroflex
Copy link
Collaborator Author

So right now there are live develop and latest branches which means anytime there's a push on them, the docs for those branches are built. There's also the option to activate a live pre/2.* branch too. I've emailed to chat further with rtd, but if it is a thing of guaranteeing build resources then we might only want to have the latest branch live. There might be issues if we push the stable and latest branch at the same time which we do when we release so would have to think about that. If our documentation grows to be too big, it's not inconceivable we'd have to evaluate if we want to manage our own docs build resources/ghactions or something like this. I've tried to make it faster but I think the documentation just has a lot of information and files.

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Aug 1, 2024

So this is a bit interesting. The same-state build actually works, but it means it needs to use all the build resources available on read the docs project. Ie no other branches can concurrently build when/if this gets merged.

image

It is slightly worrying actually, because of the risk that there comes a point the documentation doesn't build. I've emailed readthedocs about this but still. I don't like that it is a fragile build resources situation because of the risk one day, or specifically, one release - this gets screwed up and it could take days to fix depending if it's a readthedocs server problem again. The thing that concerns me, is that if we merge this, I cannot guarantee that the docs will always build with new documentation information added. I guess worst case scenario we can revert these changes.

Maybe we need to start seriously thinking of:

  • See if readthedocs can sort this out.
  • Having our own documentation build resources ie migrating away from readthedocs if they can't sort this.
  • Breaking off the documentation into multiple projects which will be a pretty major work. It should be possible to have the notebooks and faq docs build separately, but again it's a pretty big effort in making sure all the links and website loading gets added properly at the web level.

If this PR gets merged, @momchil-flex if you have some thoughts about this too because of the potential implications during a release process.

@momchil-flex
Copy link
Collaborator

Sorry, I think I'm a bit lost here. This PR does something that would require us to only build one branch at a time on readthedocs and we may soon reach a point where this doesn't work either because our build is too heavy? But we may be getting close to that anyway with or without this PR?

Readthedocs is definitely convenient to manage versions, visibility, etc. Is there a convenient way to provide our own computational resources for the builds while still using them for the rest?

I think having one active build at a time is not terrible but the danger that even that starts breaking is... not good, yeah. :)

@daquinteroflex
Copy link
Collaborator Author

The intention of this PR is to enable us to build the API classes with all the documented members as it used to be before, per user requests. However, I think between last time we did a full build and now, the amount of content within the documentation has increased a lot. This means that the full API build takes a long time locally, and I've done everything I could find to speed it up. However, when we went to build this ifull-api version in the server, the docs began to fail building due to timeout errors. I fought this for a few weeks now, and got in touch with read the docs support, who increased the server configuration for our project so it could actually begin building this. Now, the docs are taking >2.5 hrs to build each time on this PR in read the docs, but they are building.

The issue I recently found is that despite it being able to build, if there is another branch or another concurrent build, such as stable and latest building at the same time - then the build will fail because the readthedocs servers error. If a single branch build is done individually, then the build does complete. I've emailed rtd about this already.

On your comment, the concern is exactly as you describe it. It's the unreliability of this that concerns me particularly and as the project files increase it is unclear to me how this will handle it. As we debug with them, will propose the idea of using our own computation resources but using them to manage the documentation.

@momchil-flex
Copy link
Collaborator

The intention of this PR is to enable us to build the API classes with all the documented members as it used to be before, per user requests.

Have we really thought this through? I remember the documentation used to look quite cluttered. There will always be user requests, we need to make sure we're addressing the ones that make sense. :)

2.5h is pretty crazy. Any idea if it is possible to speed up with more cpu resources if we do a local build?

No need to say this will only be getting worse... In the end going to a modular documentation may be inevitable. I understand it's unreasonable for you to this now though so it may have to either be someone else who picks this up, or we try to survive until next year. :)

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Aug 1, 2024

Ultimately, I think the goal is to implement all the documentation functionality we want in our new sphinx extension flexcompute/autoflex#12 . But because that will take some iterating to get right, the interim approach that made sense at the time was to introduce the full api build with all inherited methods until the sphinx extension was ready.

It should be possible to reduce some of the build time by doing the _autosummary locally, but then this has to be deleted and recommited every time. It should reduce some build resources I think and can attempt to automate this. The cpu or gpu resource management is configured by reathedocs, but still need to explore with them about using our own resources.

However, I think maybe what I've learnt from the actual implementation challenges for this interim PR supersede the intended functionality as you describe. Because the docs build situation is unstable if we implement the interim rolled-back inherited api build, as only really seems clear after rtd modified their server configuration as it has occured previously, we would be shooting ourselves in the foot in the long run on implementing this if read the docs cannot sort this out.

Rather than focus on interim solutions, it seems in the long run, it makes sense just to focus on implementing this desired functionality in the autoflex extension. However, am afraid, after all the attempts I've had at sorting this out in a patch we could roll out sooner, it seems unreasonable to continue to follow this full api approach given the issues that have appeared and I can't think of a feasible approach to sort this out sooner without major changes @tylerflex @tomflexcompute . If you have any suggestions, would be down to explore that or otherwise will just focus on properly developing this in the autoflex extension.

@daquinteroflex
Copy link
Collaborator Author

Closed in favour of #2009 for now.

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.

3 participants