Documentation upgrade after big rebase #1320
Conversation
There was a problem hiding this comment.
@daquintero this is an insane amount of work, super excited to get it merged. All of my comments are pretty minor and most are things we can probably work on later. So feel free to work on as many as you want before we take another look Monday.
And if you want people from MC or the EM team to take a look feel free to assign other reviewers.
| black . --check --diff | ||
| ruff check tidy3d --fix --exit-non-zero-on-fix | ||
| pytest -rA tests | ||
| pytest -rA tests/test_data/_test_datasets_no_vtk.py |
There was a problem hiding this comment.
let's figure out what to do about this, if we want to keep it in the tests, we can remove the underscore.
There was a problem hiding this comment.
Sounds good! I think they're related to @dbochkov-flexcompute heat/vtk addition? What do you think Daniil?
There was a problem hiding this comment.
This tests that everything works as expected if vtk package is not installed. _test_datasets_no_vtk.py contains a mock for vtk import failure, but I couldn't make it work if it runs together with the other tests (vtk gets imported in other tests and I am not sure how to properly unload it). Thus, I added underscore to excluded it from all other tests and run it in a separate pytest execution. Is there a nicer way to accomplish this?
| with: | ||
| folder: build_toc | ||
| token: ${{ secrets.GH_PAT }} | ||
| repository-name: flexcompute/tidy3d-example-library |
There was a problem hiding this comment.
does this repo exist? is it supposed to be here? https://github.com/flexcompute/tidy3d-example-library or is it referring to the website https://www.flexcompute.com/tidy3d-example-library/
There was a problem hiding this comment.
So this is related to the new workflow in terms of the syncing the new notebooks repo, but without affecting the existing MC setup. I understand https://github.com/flexcompute/tidy3d-example-library is what is used to host https://www.flexcompute.com/tidy3d-example-library/. This github action came from the tidy3d-docs repo, which was basically automatically filtering out stuff not needed to render the notebooks. I've modified it so that it gets updated in the same way in order not to break compatibility, but in reality MC could use the new development tidy3d-notebooks repo which is equivalent rather than this proxy repo.
In any case, this should be an equivalent github action workflow to what existed before in the tidy3d-docs repo. I'm not sure which is Yongwei's github handle. Is it @magiWei maybe wants to look into it? This is related to the message I sent you a few weeks ago and just including if it's handy?
Hi Yongwei, hope you've been well!
I have good news that maybe you find interesting. We have the new documentation demo up and running. Hope you like it. We're getting prepared to eventually implement this in the 2.6 release and it would be great to get you onboard with the plan and solve any issues to make sure we're happy with it.
The summary of the implementation is here:
Currently the demonstrated demo implemented flow goes as follows:
Development occurs in tidy3d/repo_merge_no_history. The sync-to-readthedocs-repo action synchronises the main repository with flexcompute-readthedocs/tidy3d-docs-demo which can be built and hosted by readthedocs without access to the flexcompute organisation. This demo is set up with the corresponding flexcompute/tidy3d-notebooks/develop, so that the docs build and pull this github submodule. This is what I aim to replicate with the main repositories & branches.
Summary of changes:
- tidy3d consolidates docs + source for development.
- tidy3d-notebooks is the new sole directory of notebooks development. (This could replace tidy3d-example-notebooks if we wanted too)
- The easiest future for of tidy3d-docs for everyone could be that it just becomes a mirror repo to tidy3d like in the implementation I describe above. However, because the repository had to have its directories restructured, I've done some modifications to the sync and sync-to-proxy-repo Github actions. I suspect there are likely to be transition issues in the implementation which we can solve together. It's a bit tricky to test them without running them, and it'd be great to get your input and feedback here.
I've discussed the caveats of the implementation plan here if you want a full explanation.
tidy3d/web/cli/develop.py
Outdated
There was a problem hiding this comment.
please add docstrings to these fns
|
Some more thoughts:
|
|
just FYI, I got this when running did you want to pass the kwargs to def echo_and_run_subprocess(command: list, **kwargs):
concatenated_command = " ".join(command)
print("Running: " + concatenated_command)
return subprocess.run(command, **kwargs)After this, it's breaking because |
|
yea anytime I try to |
So I thought about this too. The issue is that the
... So I've created a new command that should be reproducible but for reference the command style is:
Hmm this is new to me! I wonder if it could be a python 3.12 thing, since I have not tested with that, shall I? I had in the tests to support till 3.11? EDIT: I've checked that the command you ran was in 3.10, weird. Maybe this is a second effect of my installation commands, since you have two poetry versions installed, in different python versions. Hmm I will have a think how to make this more robust. It's weird since the command checks poetry is not installed before installing it in homebrew. But surely something weird is happening. I'm going to edit that the poetry installation should use the python version that was used to install it, ie the one on the terminal you ran, rather than a system default. Maybe this helps on making it more reproducible as potentially the issue is that it is using a python on the system rather than your venv. Could also be related to this issue |
|
I tried again with |
|
I'm currently writing a |
|
Shall we give this a try? Works on ubuntu pip install -e .
tidy3d develop uninstall-dev-envrionment
tidy3d develop install-dev-environment
poetry run tidy3d develop verify-dev-environment |
|
Just to update:
In any case, what this means, is that those actions get deployed from a different repo to this one, so merging this should not affect the github actions fundamentally. |
daquinteroflex
force-pushed
the
repo_merge_no_history
branch
from
f86ed93
to
47bef21
Compare
Currently verifying what has happened