Skip to content

Fixing UMA test#27

Merged
cplett merged 4 commits intofaccts:mainfrom
cplett:bugfix.uma_test
Dec 17, 2025
Merged

Fixing UMA test#27
cplett merged 4 commits intofaccts:mainfrom
cplett:bugfix.uma_test

Conversation

@cplett
Copy link
Contributor

@cplett cplett commented Dec 10, 2025

When the UMA model files are not downloaded beforehand, the UMA test will often fail due to the 10-second timeout. This PR ensures that the model files are automatically downloaded during server setup if they are missing. This prevents the timeout and allows the UMA test to run reliably.

Signed-off-by: Christoph Plett <plett@faccts.de>
@cplett cplett requested a review from stgeo December 10, 2025 14:51
@cplett cplett self-assigned this Dec 10, 2025
@cplett cplett added the bug Something isn't working label Dec 10, 2025
Copy link
Contributor

@stgeo stgeo left a comment

Choose a reason for hiding this comment

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

What if the download hangs - not sure there is a timeout somewhere inside get_predict_unit. I would just rather the timeout here to some reasonable download time - 60s? 120s? - but have to draw the line somewhere.

Signed-off-by: Christoph Plett <plett@faccts.de>
@cplett cplett requested a review from stgeo December 16, 2025 09:56
Copy link
Contributor

@stgeo stgeo left a comment

Choose a reason for hiding this comment

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

Wow, that was more infrastructure than I expected - thanks for taking the time!

I have a few minor corrections in the test_utilities and some general points about the way the setup steps are done. You don't necessarily need to change all these now, but please respond with your thoughts and perhaps open issues for later.

Signed-off-by: Christoph Plett <plett@faccts.de>
@cplett cplett requested a review from stgeo December 16, 2025 14:51
Copy link
Contributor

@stgeo stgeo left a comment

Choose a reason for hiding this comment

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

Thanks! Some small final things and you can go ahead and merge.

uma_model = "uma-s-1p1"


def cache_model_files(
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this could go to oet.calculator.uma to avoid the code duplication. But we probably need another round of streamlining of the test infrastructure anyway, so no big deal for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the test infrastructure requires streamlining at some point so I leave it for now. Thanks!

Signed-off-by: Christoph Plett <plett@faccts.de>
@cplett cplett merged commit 9c25bc8 into faccts:main Dec 17, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants