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 #20

Closed
ablaom opened this issue Sep 6, 2023 · 3 comments · Fixed by #30
Closed

Improve local testability #20

ablaom opened this issue Sep 6, 2023 · 3 comments · Fixed by #30
Assignees
Labels
enhancement New feature or request

Comments

@ablaom
Copy link
Member

ablaom commented Sep 6, 2023

Running Pkg.test("MLJFlow") locally requires that an MLflow service is already running on your machine and that the uri "http://localhost:5000" will work to connect to it. On my mac, that uri will not work and I must manually edit it to be "http://127.0.1:5000", which is a pain.

Here's one suggestion: To run tests one must set a local env variable "MLFLOW_URI" to the uri of an active MLflow service. If the env is empty a helpful warning explaining what to do is thrown.

@pebeto Do you have any other suggestions?

@pebeto
Copy link
Member

pebeto commented Sep 28, 2023

In this case, are we going to set a default value for MLFLOW_URI?

I consider this change like something practical, but it can be tedious for the end-user. However, not all of us have mljflow running on the same port, so I think it's good to go.

@pebeto pebeto self-assigned this Sep 28, 2023
@pebeto pebeto added the enhancement New feature or request label Sep 28, 2023
@ablaom
Copy link
Member Author

ablaom commented Sep 28, 2023

| In this case, are we going to set a default value for MLFLOW_URI?

The problem with setting a default is this: If we get an error, how do we know it's because the URI's don't match and not for some other reason?

@pebeto pebeto closed this as completed Oct 13, 2023
@ablaom
Copy link
Member Author

ablaom commented Oct 16, 2023

Reopening as not complete. We need to:

  • Change the URI's in tests to the one set by the environment var. There at two places, here is one:

    logger = MLJFlow.Logger("http://localhost:5000")

  • Set the env variable appropriately in CI.yml

@pebeto In the future, can we please have reviewable PR's for new changes, rather than direct commits to dev?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants