feat: NoAuth for AML mlflow Logging#200
Conversation
|
This seems legitimate to allow a different type of authentication. Also, could you please refactor the code to avoid inheriting from concrete class, maybe creating an additional generic class? Or maybe calling your class "GenericAuth" and making "TokenAuth" inherit from it? |
|
Thanks and that all sounds good to me @floriankrb. I'm currently taking some leave this week, but I'll make those changes next week when I'm back. It shouldn't take much but I just want to test that something doesn't go wrong. |
There was a problem hiding this comment.
Thanks for this, agreed with Florian's suggestions. I propose something like:
AuthBase: ABC with init, save, login and authenticate.NoAuth(AuthBase): Your class that does nothing.TokenAuth(AuthBase)
I originally went with Auth instead of Authentication because that's consistent with naming of similar components in other web packages like requests. But if we like Authentication more feel free to change.
I'd also add a test - it's a bit silly for a class that does nothing but just in case.
b33268b to
dc92e82
Compare
|
Hi @floriankrb & @gmertes, thanks for the comments on this one. I updated the NoAuth class as suggested:
This works for my purposes, please let me know if you'd like anything else to go in the PR. Thanks! |
🤖 Automated Release PR This PR was created by `release-please` to prepare the next release. Once merged: 1. A new version tag will be created 2. A GitHub release will be published 3. The changelog will be updated Changes to be included in the next release: --- ## [0.4.36](0.4.35...0.4.36) (2025-09-22) ### Features * Add aliases to registry ([#219](#219)) ([37267b5](37267b5)) * Debug imports ([#182](#182)) ([1eaa615](1eaa615)) * NoAuth for AML mlflow Logging ([#200](#200)) ([732182e](732182e)) * Rich logging ([#209](#209)) ([3c762a5](3c762a5)) * Speedup checkpoint editing - remove compression ([#218](#218)) ([b49120f](b49120f)) * Use obstore to access s3 buckets ([#210](#210)) ([da380be](da380be)) ### Bug Fixes * Add missing s3 function used by datasets ([#212](#212)) ([30589e8](30589e8)) --- > [!IMPORTANT] > Please do not change the PR title, manifest file, or any other automatically generated content in this PR unless you understand the implications. Changes here can break the release process. >⚠️ Merging this PR will: > - Create a new release > - Trigger deployment pipelines > - Update package versions **Before merging:** - Ensure all tests pass - Review the changelog carefully - Get required approvals [Release-please documentation](https://github.com/googleapis/release-please)
## Description I have been logging metrics to Azure ML, which requires Azure's special blend of mlflow. This PR adds those features, which I think might be useful to a broader audience of people who either run on Azure, Azure ML, or simply log and view metrics on AML. I'm currently running on an on-prem HPC machine but logging to AML with this code, however I think it will be necessary for those actually running on AML as well. cc @mchantry ## What problem does this change solve? Using mlflow to log to Azure ML requires some azure-specific code and packages. ## What issue or task does this change relate to? No open issue, please let me know if you'd like me to open one up to describe the situation, I'd be happy to do so. ## Additional notes ## I got this working *just enough* to my liking, but there are still some issues unfortunately. * tests: I'm not sure how to make tests with this, since it would require logging to AML... If anyone has any ideas I'm all ears * TokenAuth: is not necessary with AML, see necessary helper PR at ecmwf/anemoi-utils#200 * terminal logs: with regular mlflow, terminal logs get updated by simply overwriting the terminal_log.txt. Overwriting files is not allowed with azure-mlflow. Moreover this appears to happen on each process (?) that's running, so many times over and over. I put in some try/except logic to try writing these files, but just pass whenever a failure happens. This means the terminal_log.txt stops updating pretty early on in the training process. I'm not really sure what to recommend here. * diagnostic plots: these also seem to get written by each process (worker?). However, since each unique plot has a unique name, putting the try/except logic here doesn't really matter - each process tries to log the plot and if it succeeds, that means it was the first one and the plot gets stored. If the process fails, then it just moves on passively since a different one beat it to the job. no problem but maybe not the cleanest solution. * model checkpoints: I simply could not get model checkpoints to log to AML, and I don't know what is going on there. For my purposes, I don't really care though, since they get stored locally on the machine where I will use the checkpoints anyway. Also, @mariahpope at EPIC/NOAA has been kicking off some anemoi training runs on AML, and noticed the checkpoints are stored. So, this may not be a problem. * hyperparameter logging: in the second half of the method `log_hyperparams_in_mlflow`, the logger tries to log "expanded_params" after truncating them. I have no idea why but I could not get this to work for AML, despite lots of attempts with ChatGPT and Microsoft Copilot. This is the reason for overriding this method in the Azure subclass. * I think there are a lot of different ways to establish a connection to an AML workspace, I did it here with the following code block: ```python sp_auth = ServicePrincipalAuthentication( tenant_id=os.environ["AZURE_TENANT_ID"], service_principal_id=os.environ["AZURE_CLIENT_ID"], service_principal_password=os.environ["AZURE_CLIENT_SECRET"], ) ws = Workspace( subscription_id=os.environ["AZURE_SUBSCRIPTION_ID"], resource_group=aml_resource_group, workspace_name=aml_workspace_name, auth=sp_auth, ) ``` Note there's a check before those lines to make sure the necessary environment variables are defined for the user. I would be very open to making this more flexible based on what true AML users think, or we could let this evolve as more users start to flex this feature. Let me know if these limitations are acceptable, and I'd be happy to put some warning flags for the user so it's clear what will and will not be logged to AML. If anyone wants these to be worked out in the PR, I'd love to hear some suggestions - I'm pretty new to AML and leaned heavily on some LLMs to make this work, but "we" all got stuck on these issues :) --- ***As a contributor to the Anemoi framework, please ensure that your changes include unit tests, updates to any affected dependencies and documentation, and have been tested in a parallel setting (i.e., with multiple GPUs). As a reviewer, you are also responsible for verifying these aspects and requesting changes if they are not adequately addressed. For guidelines about those please refer to https://anemoi.readthedocs.io/en/latest/*** By opening this pull request, I affirm that all authors agree to the [Contributor License Agreement.](https://github.com/ecmwf/codex/blob/main/Legal/contributor_license_agreement.md) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Nathan Simpson <[email protected]> Co-authored-by: Nathan Simpson <[email protected]> Co-authored-by: Nathan Simpson <[email protected]> Co-authored-by: Ana Prieto Nemesio <[email protected]>
Description
This is a support PR for ecmwf/anemoi-core#469. My basic understanding is that the authentication processes are applicable to AML mlflow logging, since lots of credentials are required to setup the workspace in the first place. So, this creates a PassiveAuth authenticator to allow the AnemoiAzureMlflowLogger to be a child class of the AnemoiMlflowLogger parent class, just silently ignoring all the TokenAuth steps.
What problem does this change solve?
New feature: supports AML mlflow logging.
What issue or task does this change relate to?
None open but happy to open one if necessary.
Additional notes
See ecmwf/anemoi-core#469 discussion
As a contributor to the Anemoi framework, please ensure that your changes include unit tests, updates to any affected dependencies and documentation, and have been tested in a parallel setting (i.e., with multiple GPUs). As a reviewer, you are also responsible for verifying these aspects and requesting changes if they are not adequately addressed. For guidelines about those please refer to https://anemoi.readthedocs.io/en/latest/
By opening this pull request, I affirm that all authors agree to the Contributor License Agreement.