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

BUG: Fix for duplicate authentication #878

Merged
merged 40 commits into from
May 16, 2023
Merged

BUG: Fix for duplicate authentication #878

merged 40 commits into from
May 16, 2023

Conversation

ant0nsc
Copy link
Collaborator

@ant0nsc ant0nsc commented Apr 28, 2023

Resolve issues with AML SDK v1/v2: Even in the v2 code path we are creating v1 workspaces, leading to duplicate authentication requests.
To achieve that, a couple of other changes were necessary:

  • Deprecate the use of the default datastore, which was read out of an SDK v1 Workspace object even if SDK v2 was chosen
  • No longer allowing SDK v2 when mounting datasets for local runs (v2 Datasets can't be mounted at all).

Also added more detailed logging for dataset creation, and a commandline flag to control logging level.

@ant0nsc ant0nsc linked an issue Apr 28, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #878 (46ac0db) into main (683def9) will increase coverage by 0.32%.
The diff coverage is 44.44%.

Impacted file tree graph

Flag Coverage Δ
hi-ml 83.28% <83.33%> (+0.26%) ⬆️
hi-ml-azure 26.99% <30.18%> (+1.60%) ⬆️
hi-ml-cpath 75.91% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hi-ml-azure/src/health_azure/himl_download.py 0.00% <ø> (ø)
hi-ml-azure/src/health_azure/logging.py 0.00% <ø> (ø)
hi-ml-azure/src/health_azure/datasets.py 27.39% <22.22%> (-8.63%) ⬇️
hi-ml-azure/src/health_azure/utils.py 29.39% <25.64%> (+0.58%) ⬆️
hi-ml/src/health_ml/runner.py 86.20% <57.14%> (-0.68%) ⬇️
hi-ml-azure/src/health_azure/himl.py 41.32% <80.00%> (+14.30%) ⬆️
...th/src/health_cpath/scripts/mount_azure_dataset.py 70.37% <100.00%> (ø)
hi-ml/src/health_ml/experiment_config.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

raise ValueError(f"Couldn't connect to MLClient: {e}")
logging.info(f"Logged into AzureML workspace {ml_client.workspace_name}")
return ml_client
logger.info(f"Using MLClient for AzureML workspace {workspace_name} as specified by environment variables")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text.
@fepegar fepegar self-requested a review May 9, 2023 10:31
@fepegar
Copy link
Contributor

fepegar commented May 9, 2023

Can you please summarise the changes in this PR so it's a bit easier to review?

Copy link
Contributor

@kenza-bouzid kenza-bouzid left a comment

Choose a reason for hiding this comment

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

LGTM, minor suggestions and Qs

hi-ml-azure/src/health_azure/datasets.py Outdated Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Outdated Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Show resolved Hide resolved
hi-ml-azure/src/health_azure/utils.py Show resolved Hide resolved
hi-ml-azure/src/health_azure/utils.py Outdated Show resolved Hide resolved
hi-ml/src/health_ml/experiment_config.py Outdated Show resolved Hide resolved
hi-ml/src/health_ml/runner.py Show resolved Hide resolved
@ant0nsc ant0nsc requested a review from fepegar May 10, 2023 20:26
@fepegar
Copy link
Contributor

fepegar commented May 11, 2023

I will take a look at this tomorrow.

Copy link
Contributor

@fepegar fepegar left a comment

Choose a reason for hiding this comment

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

LGTM, although I'm not very confident. I've left some minor comments.

hi-ml/src/health_ml/experiment_config.py Outdated Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Outdated Show resolved Hide resolved
hi-ml-azure/src/health_azure/himl.py Outdated Show resolved Hide resolved
hi-ml-azure/src/health_azure/utils.py Outdated Show resolved Hide resolved
hi-ml-azure/testazure/testazure/test_get_ml_client.py Outdated Show resolved Hide resolved
@ant0nsc ant0nsc merged commit f46f60e into main May 16, 2023
@ant0nsc ant0nsc deleted the antonsc/duplicate_auth branch May 16, 2023 19:26
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.

When submitting a job via SDK v2, some users have to authenticate twice
3 participants