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

Add add_image method to Dataset entity #389

Merged
merged 8 commits into from
May 21, 2024
Merged

Add add_image method to Dataset entity #389

merged 8 commits into from
May 21, 2024

Conversation

crisely09
Copy link
Contributor

@crisely09 crisely09 commented Feb 26, 2024

To tackle #388

This is the simplest implementation to "attach" and image, and then use it to add it to any resource, similar to attach for distributions.

Example of how it is used can be found inside the 03 - Storing notebook.
The way to add the image would be :

image = forge.attach_image(path, content_type, about)
resource.image = image

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 75.65%. Comparing base (be27bb6) to head (a391825).

Files Patch % Lines
kgforge/core/archetypes/store.py 25.00% 3 Missing ⚠️
kgforge/specializations/resources/datasets.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
- Coverage   75.69%   75.65%   -0.05%     
==========================================
  Files         103      103              
  Lines        6753     6761       +8     
==========================================
+ Hits         5112     5115       +3     
- Misses       1641     1646       +5     
Flag Coverage Δ
unittests 75.65% <37.50%> (-0.05%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eleftherioszisis
Copy link

eleftherioszisis commented Feb 27, 2024

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

@MFSY
Copy link
Collaborator

MFSY commented Feb 27, 2024

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance. It is okay to add more data models for specific properties we want to offer as default on Resource or on one of its descendants.

forge.attach already supports uploading a dir (you'll get a list of distributions DataDownload)

@crisely09
Copy link
Contributor Author

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance.

@MFSY my issue with adding it to Resource is that we would need to pass forge to it, as it is done in Dataset, and that is kind of ugly. Also because that would mean a big change in how we have been initializing Resource.
Maybe passing forge only to the method add_image ?

@crisely09
Copy link
Contributor Author

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

@MFSY
Copy link
Collaborator

MFSY commented Feb 27, 2024

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance.

@MFSY my issue with adding it to Resource is that we would need to pass forge to it, as it is done in Dataset, and that is kind of ugly. Also because that would mean a big change in how we have been initializing Resource. Maybe passing forge only to the method add_image ?

I am okay with having the method accept a forge object. Now Resource can also be extended with an Entity class that will have this method which can accept forge. In that case, client will need to create an Entity from a resource before using the method.

@AurelienJaquier
Copy link

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

What if we have files from different directories? Would the piece of code provided by elefterios work?

@crisely09
Copy link
Contributor Author

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

What if we have files from different directories? Would the piece of code provided by elefterios work?

I think it should, yes

@darshanmandge
Copy link

Hi @crisely09!
Can this PR be merged now?

@stefanoantonel
Copy link

Any update on this merge?

@MFSY
Copy link
Collaborator

MFSY commented May 21, 2024

Hi guys,
We'll look into this this week.

@crisely09
Copy link
Contributor Author

With the current changes, to add an image, one has to start from a dataset:

dataset = Dataset.from_resource(forge, resource)
dataset.add_image(path, content_type, about)

This should work in the same way as explained before for paths and for a list of files.

@crisely09 crisely09 changed the title Add attach_image method to forge Add add_image method to Dataset entity May 21, 2024
@crisely09 crisely09 merged commit 4231772 into master May 21, 2024
1 check passed
@crisely09 crisely09 deleted the add_image branch May 21, 2024 12:58
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.

None yet

7 participants