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

Feature/get actual data from AI Assets #175

Merged
merged 8 commits into from
Nov 9, 2023
Merged

Conversation

jsmatias
Copy link
Member

@jsmatias jsmatias commented Nov 1, 2023

The class ResourceAIAssetRouter was created inheriting ResourceRouter.
The create method in the new class adds two new routes to the APIRouter:
1. f"{url_prefix}/{self.resource_name_plural}/{version}/{{identifier}}/data"
2. {url_prefix}/{self.resource_name_plural}/{version}/{{identifier}}/data/{{distribution_idx}}"

The get_resource_data_func returns a function to get the actual data from an AI asset. Setting the default parameter as False, it returns the get_resource_data function, which accepts the index of the distribution (i.e. 0, 1, 2, etc. for cases with multiple distribution). If default is set to True it returns the get_resource_data_default, which always returns the first item of the distribution list (i.e. index 0).

Finally, the AI assets: datasets, ml_models, experiments, and case_studies were modified to inherit ResourceAIAssetRouter, instead of the previous ResourceRouter.

… the method to append endpoints for downloading the actual data

 On branch feature/get-actual-data
 Changes to be committed:
	new file:   src/routers/resource_ai_asset_router.py
	modified:   src/routers/resource_router.py
	modified:   src/routers/resource_routers/dataset_router.py
 On branch feature/get-actual-data
 Changes to be committed:
	modified:   src/routers/resource_ai_asset_router.py
	modified:   src/routers/resource_routers/case_study_router.py
	modified:   src/routers/resource_routers/experiment_router.py
	modified:   src/routers/resource_routers/ml_model_router.py
	modified:   src/routers/resource_routers/publication_router.py
	new file:   src/tests/routers/resource_routers/test_router_case_study_retrieve_data.py
	new file:   src/tests/routers/resource_routers/test_router_dataset_retrieve_data.py
	new file:   src/tests/routers/resource_routers/test_router_experiment_retrieve_data.py
	new file:   src/tests/routers/resource_routers/test_router_ml_model_retrieve_data.py
	new file:   src/tests/routers/resource_routers/test_router_publication_retrieve_data.py
@jsmatias jsmatias added the enhancement New feature or request label Nov 1, 2023
@jsmatias jsmatias self-assigned this Nov 1, 2023
Copy link

@josvandervelde josvandervelde left a comment

Choose a reason for hiding this comment

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

Looks good! I like the attention to detail (e.g. the response headers).
I added some minor comments, only the unittests can use some real attention, I think.

src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
src/routers/resource_ai_asset_router.py Outdated Show resolved Hide resolved
…distributions and improved the structure of the automated tests

 Changes to be committed:
	modified:   src/routers/resource_ai_asset_router.py
	modified:   src/routers/resource_router.py

	new file:   src/tests/resources/contents/example1.csv
	new file:   src/tests/resources/contents/example2.tsv
	new file:   src/tests/routers/ai_asset_routers/__init__.py
	new file:   src/tests/routers/ai_asset_routers/test_router_aiassets_retrieve_content.py
	deleted:    src/tests/routers/resource_routers/test_router_case_study_retrieve_data.py
	deleted:    src/tests/routers/resource_routers/test_router_dataset_retrieve_data.py
	deleted:    src/tests/routers/resource_routers/test_router_experiment_retrieve_data.py
	deleted:    src/tests/routers/resource_routers/test_router_ml_model_retrieve_data.py
	deleted:    src/tests/routers/resource_routers/test_router_publication_retrieve_data.py
Copy link

@josvandervelde josvandervelde left a comment

Choose a reason for hiding this comment

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

Great! Just one minor detail

"Content-Disposition": (
"attachment; " f"filename={filename or url.split('/')[-1]}"
),
"Content-Type": f"{encoding_format or 'unknown'}",

Choose a reason for hiding this comment

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

"unknown" is not a valid mimetype.
https://stackoverflow.com/questions/1176022/unknown-file-type-mime suggests to remove the content-type in this case, that sounds like a good approach to me!

Copy link

@josvandervelde josvandervelde left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@jsmatias jsmatias merged commit 044a3e3 into develop Nov 9, 2023
1 check passed
@andrejridzik andrejridzik deleted the feature/get-actual-data branch October 29, 2024 13:26
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 this pull request may close these issues.

2 participants