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

Improve local testability #31

Open
ablaom opened this issue Sep 6, 2023 · 7 comments
Open

Improve local testability #31

ablaom opened this issue Sep 6, 2023 · 7 comments

Comments

@ablaom
Copy link
Member

ablaom commented Sep 6, 2023

Testing this package in the usual way, on one's local machine, does not work:

using Pkg
Pkg.activate(temp=true)
Pkg.add("MLFlowClient")
Pkg.test("MLFlowClient")

     Testing Running tests...
createexperiment: Error During Test at /Users/anthony/.julia/packages/MLFlowClient/Szkbv/test/test_experiments.jl:1
  Got exception outside of a @test
  HTTP.Exceptions.StatusError(403, "POST", "/api/2.0/mlflow/experiments/create", HTTP.Messages.Response:
  """
  HTTP/1.1 403 Forbidden
  Content-Length: 0
  Server: AirTunes/620.8.2

Obviously the error thrown in not helpful. I expect one needs an active MLflow service running on your machine, and that it must have the appropriate uri.

Here is a related issue with a suggestion for remedy: JuliaAI/MLJFlow.jl#20

@pebeto
Copy link
Member

pebeto commented Jan 4, 2024

@ablaom do you think is a good idea to replicate the environment variable approach implemented in MLJFlow testing suite? I think it can ensure the user to run all the tests correctly.

@ablaom
Copy link
Member Author

ablaom commented Jan 4, 2024

Sure, unless you have a better suggestion.

@stemann
Copy link
Contributor

stemann commented Apr 8, 2024

#38 should do what has been discussed.

@deyandyankov
Copy link
Collaborator

When the initial development happened, the assumption was that the developer would have a local mlflow instance running prior to executing the tests. Moreover, it was also up to restart (wipe out) the mlflow instance prior to running the tests another time. This helped with debugging (i.e. final state remains in mlflow if tests fail and developer can debug) and it was also meaningful because actual functionality was tested against an actual instance. The same setup we have in the github CI job - the mlflow instance is actually a separate container that is spun up alongside the test container.

Another way to go would be to mock the mlflow endpoints. This will not require an mlflow instance at all. That's probably the correct way in general and it will allow Pkg.test() to execute without any service dependencies.

@ablaom
Copy link
Member Author

ablaom commented Apr 11, 2024

Helpful comments, @deyandyankov, thank you.

Another way to go would be to mock the mlflow endpoints. This will not require an mlflow instance at all. That's probably the correct way in general and it will allow Pkg.test() to execute without any service dependencies.

Do you mean a fully functioning mock up of an mlflow service? This would have these drawbacks:

  • A bit of work to set up, no?
  • Possibility that bugs in the mock mean some wrong behaviour of MLFlowClient.jl is not caught.
  • Adds a maintenance burden to testing for breaking releases of mlflow

As maintenance on this site has become exceedingly thin, this doesn't make this realistic option just now, in my view. (Side note: I did actually created a baby mock-up for the POC at #41.)

@deyandyankov I might have missed something, but is there anything proposed in #38 that upsets the testing workflow you just described?

@pebeto
Copy link
Member

pebeto commented May 20, 2024

To improve the project testability, I suggest to use BrokenRecord.jl. It brings us a "cassette" approach to record the HTTP requests, removing the necessity of having a mlflow instance running on our local machine.

However, this can include a new environment variable USE_TEST_RECORDS, because testing using a true server is still being required for the CI job (and locally if required).

@ablaom
Copy link
Member Author

ablaom commented May 21, 2024

This sounds like a nice idea!

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

No branches or pull requests

4 participants