Skip to content

Comments

[MNT] Dockerized tests for CI runs using localhost#1629

Open
satvshr wants to merge 90 commits intoopenml:mainfrom
satvshr:i1614
Open

[MNT] Dockerized tests for CI runs using localhost#1629
satvshr wants to merge 90 commits intoopenml:mainfrom
satvshr:i1614

Conversation

@satvshr
Copy link
Contributor

@satvshr satvshr commented Jan 29, 2026

Metadata

Details

  • What does this PR implement/fix? Explain your changes.
    This PR implements the setting up of the v1 and v2 test servers in CI using docker via localhost.

PGijsbers and others added 11 commits January 20, 2026 12:35
Locally, MinIO already has more parquet files than on the test server.
Note that the previously strategy didn't work anymore if the server
returned a parquet file, which is the case for the new local setup.
This means it is not reliant on the evaluation engine processing the
dataset. Interestingly, the database state purposely seems to keep
the last task's dataset in preparation explicitly (by having
processing marked as done but having to dataset_status entry).
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.82%. Comparing base (7feb2a3) to head (18960d5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1629   +/-   ##
=======================================
  Coverage   52.82%   52.82%           
=======================================
  Files          37       37           
  Lines        4371     4371           
=======================================
  Hits         2309     2309           
  Misses       2062     2062           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@satvshr satvshr marked this pull request as ready for review January 31, 2026 16:13
@satvshr satvshr marked this pull request as draft January 31, 2026 16:14
# sed -i 's|/minio/|/data/|g' config/database/update.sh

# echo "=== Patched Update Script ==="
# cat config/database/update.sh | grep "nginx"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why extra work here? locally just running the services is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kindly ignore these, the pr isnt ready for review yet as tests are still failing and I was trying to debug tests.

openml/config.py Outdated
Comment on lines 32 to 36
if sys.platform.startswith("win"):
TEST_SERVER_URL = "http://localhost"
else:
TEST_SERVER_URL = "http://localhost:8000"

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should actually use an env variable here, please see https://github.com/openml/openml-python/pull/1629/changes#r2797509441
should be controlled by that env variable, which if not set, should default to use https://test.openml.org/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not how I plan to resolve this either, just a temporary fix to the windows issue.

@geetu040
Copy link
Collaborator

The tests are taking too long because connection_n_retries is set to 5, you can set it to 1 for this PR, to avoid delays in CI.

@satvshr
Copy link
Contributor Author

satvshr commented Feb 18, 2026

The tests are taking too long because connection_n_retries is set to 5, you can set it to 1 for this PR, to avoid delays in CI.

Will do that to prevent hold ups for other CIs in the repo, for my branch it is noticeable if a run is going to fail if it has been stuck on a single test for more than a minute.

@geetu040
Copy link
Collaborator

Will do that to prevent hold ups for other CIs in the repo, for my branch it is noticeable if a run is going to fail if it has been stuck on a single test for more than a minute.

yeah but each job in this PR still takes full 150 minutes

openml/config.py Outdated
"avoid_duplicate_runs": False,
"retry_policy": "human",
"connection_n_retries": 5,
"connection_n_retries": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this would work, since we change this again in conftest.py.
To be completely sure that this works, you can temporarily set n_retries = 1 in _api_calls.py::_send_request

f"collected from {__file__.split('/')[-1]}: {flow.flow_id}",
)

@pytest.mark.skip(reason="Pending resolution of #1657")
Copy link
Collaborator

Choose a reason for hiding this comment

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

skip these only if OPENML_USE_LOCAL_SERVICES is set to True

Copy link
Collaborator

Choose a reason for hiding this comment

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

some tests are skipped though they are not mentioned in #1657, why is that?

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 was using the failures from here

Copy link
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

I don't think any failing test is coming from this PR. It would be better to conditionally skip them and link #1657. If there is new failure message which is not already mentioned there, then please comment down the failure with the failing tests so it could be tracked there. Also if some tests are failing because of pandas, create a separate issue for that, skip and link to these then.

files: coverage.xml
token: ${{ secrets.CODECOV_TOKEN }}
fail_ci_if_error: true
verbose: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this part moved above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codecov was giving errors, I do not recall why the errors were occurring.

Comment on lines +275 to +276
if os.getenv("OPENML_USE_LOCAL_SERVICES") == "true":
openml.config.TEST_SERVER_URL = "http://localhost:8000"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would ask for you suggestion. Do you think it should be here or rather moved to config.py, since you had it there before this.
I think, having this in config.py would be better, it would also be in favor of global config architecture in #1577 and migration in #1576

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First thing that comes to mind is the fact that something pertaining to the testing env should be kept in conftest, though I have no hard opinion on this. If it is more convenient for you I can move it to config too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep it here then

@satvshr satvshr requested a review from geetu040 February 24, 2026 15:14
Copy link
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

Looks good, this has not been addressed yet #1629 (comment)

openml/config.py Outdated
"avoid_duplicate_runs": False,
"retry_policy": "human",
"connection_n_retries": 5,
"connection_n_retries": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can undo this 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.

Was just waiting to see if tests pass before reverting all changes, apologies!

md5_checksum: str | None = None,
) -> requests.Response:
n_retries = max(1, config.connection_n_retries)
n_retries = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can undo this 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.

[MNT] Intermediate test plan

4 participants