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

Rename MLFlowLogger ? #23

Closed
ablaom opened this issue Sep 11, 2023 · 3 comments
Closed

Rename MLFlowLogger ? #23

ablaom opened this issue Sep 11, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@ablaom
Copy link
Member

ablaom commented Sep 11, 2023

I think having MLJFlow, MLFlowLogger, etc, - sometimes there is a "j" and sometimes not - is bit of cognitive burden. What if we rename MLFlowLogger to Logger? So, in MLJ the user does

logger = Logger(...)

In the rare case there are multiple logging platforms at play, the user can resolve the ambiguity with MLJFlow.Logger. Or we could just not export the name and have the user do logger = MLJFlow.Logger(...). Ie., the user does

logger = MLJFlow.Logger()

Of course this is a breaking change but I think that's fine at this early stage of development.

@pebeto What do you think?

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

pebeto commented Oct 13, 2023

@ablaom The changes are now on dev. You can review them here fbd8639.

How the user will use Logger constructor without the explicit export? Considering that this doesn't work in the way of MLJ.Logger.

@ablaom
Copy link
Member Author

ablaom commented Oct 16, 2023

Okay, then I take it you agree to the suggestion.

How the user will use Logger constructor without the explicit export? Considering that this doesn't work in the way of MLJ.Logger.

The user will do logger = MLJFlow.Logger(). This means we need MLJ to export the name MLJFlow, which I see does not currently happen.

@ablaom
Copy link
Member Author

ablaom commented Nov 20, 2023

This means we need MLJ to export the name MLJFlow, which I see does not currently happen.

Not sure what made me say that? In latest MLJ MLJFlow it appears to be exported:

julia> using MLJ
[ Info: Precompiling MLJ [add582a8-e3ab-11e8-2d5e-e98b27df1bc7]

julia> MLJFlow
MLJFlow

(jl_dz4xQ2) pkg> st
Status `/private/var/folders/4n/gvbmlhdc8xj973001s6vdyw00000gq/T/jl_dz4xQ2/Project.toml`
  [add582a8] MLJ v0.20.1

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

No branches or pull requests

2 participants