Skip to content

added arguments to azuremldataasset for better control of output path #75

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.

Sign up for GitHub

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

Open
Gabriel (Gabriel2409) wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

added arguments to azuremldataasset for better control of output path #75

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d507682
added arguments to azuremldataasset in order to be able to specify th…
Gabriel2409 Sep 14, 2023
47217de
removed checking if the output path exists in --az-output:
Gabriel2409 Sep 15, 2023
8d9ec10
added support for uri_file output
Gabriel2409 Sep 15, 2023
deae7c9
injection of the job id in the output path
Gabriel2409 Sep 16, 2023
ebdc850
added description for new arguments + removed versioned arg in examples
Gabriel2409 Sep 16, 2023
dd897d2
changed output path for azureml dataasset
Gabriel2409 Sep 16, 2023
0565423
Merge branch 'develop' into feature/specify-dataasset-output-path
Gabriel2409 Oct 3, 2023
07bb749
Add tests for generated Output command
Gabriel2409 Oct 4, 2023
1197c48
Update changelog
Gabriel2409 Oct 4, 2023
45eface
Add validation for datastore and azureml_root_dir parameter
Gabriel2409 Oct 5, 2023
dd50aab
Fix regex exponential backtracking in _validate_azureml_root_dir
Gabriel2409 Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kedro_azureml/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def compile(
@click.option(
"--az-output",
"azure_outputs",
type=(str, click.Path(exists=True, file_okay=True, dir_okay=True)),
type=(str, click.Path(exists=False)),
multiple=True,
help="Name and path of Azure ML Pipeline output",
)
Expand Down
4 changes: 4 additions & 0 deletions kedro_azureml/datasets/asset_dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ def __init__(
self,
azureml_dataset: str,
dataset: Union[str, Type[AbstractDataSet], Dict[str, Any]],
datastore: str = "workspaceblobstore",
azureml_root_dir: str = "kedro_azureml", # maybe combine with root_dir?
root_dir: str = "data",
filepath_arg: str = "filepath",
azureml_type: AzureMLDataAssetType = "uri_folder",
Expand All @@ -93,6 +95,8 @@ def __init__(
"""
super().__init__(dataset=dataset, root_dir=root_dir, filepath_arg=filepath_arg)

self._azureml_root_dir = azureml_root_dir
self._datastore = datastore
self._azureml_dataset = azureml_dataset
self._version = version
# 1 entry for load version, 1 for save version
Expand Down
22 changes: 17 additions & 5 deletions kedro_azureml/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,24 @@ def _get_output(self, name):
if name in self.catalog.list() and isinstance(
ds := self.catalog._get_dataset(name), AzureMLAssetDataSet
):
output_path = (
f"azureml://datastores/{ds._datastore}/paths/{ds._azureml_root_dir}"

@tomasvanpottelbergh Tomas Van Pottelbergh (tomasvanpottelbergh) Oct 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We probably need to strip off trailing slashes from ds._azureml_root_dir, or use some URI parsing lib

@Gabriel2409 Gabriel (Gabriel2409) Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think an URI parsing lib may be overkill. Moreover datastore can be equal to ${{default_datastore}} and I don't know if that will pose a problem with these libraries.

However, what about ds._azureml_root_dir.strip("/") to remove both leading and trailing slash?

Another solution would be to add checks on the parameters on data set creation and throw an error if they don't match the correct format. Actually if we go this route, this is probably something we want to add to the datastore and root_dir parameter as well.

)

# versioning system: to be discussed
output_path = (
f"{output_path}/{ds._azureml_dataset}/{ds.resolve_save_version()}"

@tomasvanpottelbergh Tomas Van Pottelbergh (tomasvanpottelbergh) Sep 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The main problem I see with this approach is that this hard codes the version in the pipeline specification. This might not be an issue for a single run using kedro azureml run, but it means a rerun of that pipeline or a scheduled job created from the result of kedro azureml compile will (attempt) to save to the same location.

Ideally, the versioning would be added to the path when the node is run, but that doesn't look feasible given that the directory has to be empty for Azure ML to accept it.

I might be overlooking something, but I now feel that we won't be able to add a sensible implementation of this feature unless Azure ML accepts non-empty output directories...

)

if ds._azureml_type == "uri_file":
raise ValueError(
"AzureMLAssetDataSets with azureml_type 'uri_file' cannot be used as outputs"
)
# TODO: add versioning
return Output(type=ds._azureml_type, name=ds._azureml_dataset)
output_path = f"{output_path}/{ds._dataset_config[ds._filepath_arg]}"
# note that this will always create a new version of the dataset, even if we
# have versioned set to false.
return Output(
type=ds._azureml_type,
name=ds._azureml_dataset,
path=output_path,
)
else:
return Output(type="uri_folder")

Expand Down