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

TooTallToby tutorials: unbreak and test. #912

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

fischman
Copy link
Contributor

@fischman fischman commented Feb 23, 2025

@fischman
Copy link
Contributor Author

FYI @jdegenstein. LMK if you'd rather I not replace your PPP0110 solution (and if you have a fix in mind already).

@fischman fischman force-pushed the ttt-unbreak-and-test branch from 7b9c87f to 3e8bb44 Compare February 23, 2025 06:49
Copy link

codecov bot commented Feb 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.93%. Comparing base (dce0c5d) to head (4027664).
Report is 11 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #912   +/-   ##
=======================================
  Coverage   96.93%   96.93%           
=======================================
  Files          32       32           
  Lines        9460     9471   +11     
=======================================
+ Hits         9170     9181   +11     
  Misses        290      290           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fischman
Copy link
Contributor Author

@gumyr WDYT, worth testing TTT examples as part of automated testing?

@jdegenstein
Copy link
Collaborator

@fischman FYI TTT examples are (sort of) tested through the benchmarking workflow (in a separate python script)

@fischman
Copy link
Contributor Author

@fischman FYI TTT examples are (sort of) tested through the benchmarking workflow (in a separate python script)

Thanks for the heads-up, I was unaware of that before.
Seems suboptimal that the examples' code is duplicated in the tests/ subdirectory (because of the likelihood of drift as one or the other place is edited, and the other is forgotten, and also because edits must be duplicated). Also ttt-23-t-24-curved_support is missing from the test.

Do you prefer to keep the structure at HEAD, or are you suggesting I keep this PR moving forward and delete the test_benchmarks copy of the examples, or something else?
(maybe test_benchmarks should run test_examples?)

@jdegenstein
Copy link
Collaborator

@fischman yes I agree that having two versions is suboptimal, I was just trying to get something up and running for benchmarking purposes with some changes that were happening on the OCCT/OCP and build123d side. Yes, I would like TTT 23-T-24 to be part of the benchmarks.

Can we simply import the models from docs/ into test_benchmarks.py ? That would probably be the "cleanest" solution requiring less work to maintain long-term.

RE: PPP0110 -- I am fine with your solution, I am not married to what I created. Hopefully OCCT 7.9 will make creating this model easier.

@fischman
Copy link
Contributor Author

fischman commented Mar 3, 2025

@jdegenstein dropped the copies in the test_benchmarks.py file and now pulling from the docs copy. PTAL.

@jdegenstein
Copy link
Collaborator

@fischman looking at the way that [benchmarks] category was eliminated resulted in benchmarks trying to run then failing because of multiple threads in the tests workflow.

/opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/pytest_benchmark/logger.py:39: PytestBenchmarkWarning: Benchmarks are automatically disabled because xdist plugin is active.Benchmarks cannot be performed reliably in a parallelized environment.

If this is the way it is setup, I would prefer that an appropriate flag is passed to pytest to explicitly disable benchmarking for the tests workflow. The current behavior seems fragile to me.

@fischman
Copy link
Contributor Author

fischman commented Mar 5, 2025

@jdegenstein good catch! Fixed to exclude benchmarks from the tests workflow.

@fischman fischman force-pushed the ttt-unbreak-and-test branch from 4677ce1 to 934625b Compare March 5, 2025 00:13
fischman added 3 commits March 4, 2025 16:14
- Unbreak the three broken tutorials (fixes gumyr#848)
  - This involved a rewrite of PPP-01-10 because I already had my own
    solution to that one and I couldn't easily tell what was going
    wrong with the previous solution.
- Add assertions to all the tutorials so that non-raising means success
- Add the TTT examples to `test_examples.py` added recently for gumyr#909
- Also added sympy to development dependencies since one of the TTT
  examples uses it.
… instead use the versions from the docs/assets/ttt directory.
@fischman fischman force-pushed the ttt-unbreak-and-test branch from 5157ddb to 789ff73 Compare March 5, 2025 00:14
@fischman
Copy link
Contributor Author

fischman commented Mar 5, 2025

@gumyr @jdegenstein any idea about the CI failure? Seems unrelated to this PR.

@jdegenstein
Copy link
Collaborator

@fischman I am going to take one more quick look, I don't believe the CI failure is related to anything you did -- just a random problem with the runner I suspect.

@jdegenstein
Copy link
Collaborator

jdegenstein commented Mar 5, 2025

OK I am merging despite what appears to be a different random CI failure.

EDIT: thanks for your contribution @fischman

@jdegenstein jdegenstein merged commit 07ff964 into gumyr:dev Mar 5, 2025
19 of 20 checks passed
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.

Fix TTT tutorials: 23-T-24 Curved Support, 01-02-post-cap, 01-10-light-cap
2 participants