fix: make rocket fin example run on MacOS#541
Conversation
CLA signatures requiredThank you for your PR, we really appreciate it! Like many open-source projects, we ask that all contributors sign our Contributor License Agreement before we can accept your contribution. This only needs to be done once per contributor. You can do so by commenting the following on this pull request: @PasteurBot I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
f0a8c1e to
6cca239
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
=======================================
Coverage 77.16% 77.16%
=======================================
Files 32 32
Lines 4418 4418
Branches 728 728
=======================================
Hits 3409 3409
Misses 714 714
Partials 295 295 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark ResultsBenchmarks use a no-op Tesseract to measure pure framework overhead. 🚀 0 faster, ✅ No significant performance changes detected. Full results
|
|
I have read the CLA Document and I hereby sign the CLA |
|
Nice @nikolasborrel! I am not sure about unpinning all the requirements; in my experience, some used packages, such as pyvsita can be very fragile. I would be ok with doing it, if the respective tesseracts would be built as part of the CI similar to t-jax.
That makes a lot of sense, but are you are sure this is the case currently? Ill summon the king of hot @linusseelinger
Not entirely sure about that one either, to me this seems to be on the user side but surely it is helpful. But maybe the user has other tesseracts that are running for other use cases? |
|
@nikolasborrel I dont think tesseracts have a timeout. So I dont think this shoudl be necessary:
|
|
To me this PR highlights at least 4 (functional / UX) bugs:
To address these properly we'd need independent reproducers for each. This is not all on you @nikolasborrel, but we do need more input at least on (1) and (4) to figure out why you ran into these issues. (I could reproduce (2) already, haven't looked into (3) yet.) On top of that there are the changes to the demo itself, that is, upgrading/unpinning all dependencies and general cleanup/optimization. Those can be valuable too, but are probably best reviewed together once the bugs ☝️ are squashed. |
I did a minimal example to reproduce, but could not. So I think you are right - I thought the issue was caused by It resolved the issues I had, but must be something else. I will investigate further - stay tuned! UPDATE: cannot reproduce this issue anymore |
I cannot reproduce anymore and have reverted. I was having this issue before removing pinned versions and using the
I was under the impression that this was intended and that the solution is to reference a running Tesseract through
Cannot reproduce anymore - I have reverted the
The reason is that the shell startup files are not sourced when running the notebook (specifically on macOS) and hence The remaining issues are hence 2) and eventually 3). |
|
Re 1: I can reproduce it (just add Re 4: Why do you not have |
Turned out it was a shell issue on my side and should not happen in general on OSX. I have removed this check. However, the error message thrown when |
…ance when HTTP sessions time out (#543) #### Relevant issue or PR Stuff that cropped up in #541 #### Description of changes **Bug 1: `logger.info({})` crashes RichLogger** - `PrefixFormatter.format()` called `escape(record.msg)` without `str()` conversion first - Fix: `escape(str(record.msg))`, matching vanilla logging behavior **Bug 2: `TesseractReference` type="image" leaks containers in loops** - `atexit.register(self.teardown)` held a strong ref to `self`, preventing GC - Fix: replace with `weakref.finalize(self, engine.teardown, container_name)` **Bug 3: Stale HTTP keep-alive connections cause `ConnectionError`** - Race between urllib3's `is_connection_dropped` check and uvicorn closing idle connections - Fix: catch `ConnectionError` in `HTTPClient._request` and retry once #### Testing done New tests reproducing the bugs, fail first (before pushing fixes) then pass.
There was a problem hiding this comment.
Let's keep all deps pinned please, just update them to current versions. (This gives us at least some level of confidence that the demo keeps working.)
There was a problem hiding this comment.
Commenting here because GH rich diff doesn't allow me to add comments inline.
- What's the logging setup for? I can't see any log messages in the notebook so seems redundant?
- If you're up for it, you could try using
from_imagefordesign_tessas well since we're now passing in the bars tess via URL. Might require adding them to a shared network though (see doc: document how to usetesseract serve --networkparameter #530). - Could add a
tqdmorrichprogress bar to the optimization loop?
92c2fce to
fdb7912
Compare
fdb7912 to
438982f
Compare
Relevant issue or PR
N/A
Description of changes
Demo notebook (optimization_os.ipynb):
bars_3das a persistent container and pass it via URL reference({"type": "url", ...})instead of{"type": "image", ...}, preventing a new container being spawned on every optimization iterationdesign_tess = Tesseract.from_image("sdf_fd", network=NETWORK, network_alias="sdf_fd")Requirements — remove strict version pins across all showcase tesseracts:
sdf_fd,bars_3d: upgradepyvista(unpinned) to fixvtkwheel incompatibility with Python 3.11Platform configs:
tesseract_config.yaml: changetarget_platformfromlinux/x86_64to native to avoid QEMU emulation on Apple SiliconTesting done
NOTE: suddenly jax_fem produces NaNs - To me the inputs looks correct, maybe some dependencies have been updated...