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

Remove preview file values #1230

Merged
merged 7 commits into from
Feb 28, 2019
Merged

Remove preview file values #1230

merged 7 commits into from
Feb 28, 2019

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Feb 26, 2019

This finishes the work needed for issue #1068. Since PR #1011, preview images in the triplestore and in Sipi are obsolete. This PR removes them and changes knora-base accordingly.

  • knora-base.ttl:
  • Change API v1 and its Sipi scripts (convert_from_file.lua and convert_from_binaries.lua) so that they no longer store preview images.
  • upgrade/README.md: Add a simple convention so Knora release notes can provide scripts and instructions for updating existing data when Knora is upgraded.
  • Remove preview images in test data (using a transformation added to the TransformData utility).
  • Update docs and release notes.

Resolves #1068.

@benjamingeer
Copy link
Author

@tobiasschweizer Could you please try updating the BEOL data using the scripts in upgrade/1230-delete-previews, and let me know if you find any problems?

@tobiasschweizer
Copy link
Contributor

@benjamingeer Ok, I will try it. I think I will be able to do that tomorrow.

@tobiasschweizer
Copy link
Contributor

I think the Python load test script would also have to be adapted. It is very similar to the import process I am using.

@benjamingeer
Copy link
Author

I think the Python load test script would also have to be adapted.

Which script is that?

@tobiasschweizer
Copy link
Contributor

Which script is that?

It is the load test script written in Python. @subotic knows where it is.

@benjamingeer benjamingeer mentioned this pull request Feb 27, 2019
@tobiasschweizer
Copy link
Contributor

I assume you are getting rid of the non gui case (shared storage of Knora and Sipi) so the import process for StillImageRepresentation has to be adapted.

@benjamingeer
Copy link
Author

benjamingeer commented Feb 27, 2019

It is the load test script written in Python.

I didn't know that such a script existed.

I assume you are getting rid of the non gui case (shared storage of Knora and Sipi)

I haven't done that yet, I'll do it in a later PR.

@tobiasschweizer
Copy link
Contributor

I haven't done that yet, I'll do it in a later PR.

Ah, sorry, I was mistaken then.

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Feb 27, 2019

I didn't know that such a script existed.

We had a stall problem in Knora when doing the import and so @subotic added a load test script written in Python.

@subotic
Copy link
Collaborator

subotic commented Feb 27, 2019

The load test can be found here: https://github.com/dhlab-basel/knora-tests

It is the import-test.

Copy link
Collaborator

@subotic subotic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :-)

@subotic
Copy link
Collaborator

subotic commented Feb 27, 2019

I like the upgrade scripts :-)

@benjamingeer
Copy link
Author

I like the upgrade scripts :-)

Thanks, I was hoping you would like them. :)

@tobiasschweizer
Copy link
Contributor

Could you please try updating the BEOL data using the scripts in upgrade/1230-delete-previews, and let me know if you find any problems?

I can confirm that upgrade/1230-delete-previews/delete-preview-values.py works.

I checked as follows:

  1. I loaded beol-data.ttl with preview file values on develop
  2. I ran this SPAQRL query:
PREFIX knora-base: <http://www.knora.org/ontology/knora-base#>
select * 
FROM <http://www.knora.org/data/0801/beol>
where { 
	    
    ?file a knora-base:StillImageFileValue .
    
    FILTER NOT EXISTS {
    	?file knora-base:isPreview true .
    } 
    
} ORDER BY ?file
  1. I downloaded the result as CSV: ordered list of file value IRIs

  2. I switched to wip/1068-remove-previews

  3. I ran the script delete-preview-values.py

  4. I ran this SPARQL query:

PREFIX knora-base: <http://www.knora.org/ontology/knora-base#>
select * 
FROM <http://www.knora.org/data/0801/beol>
where { 
	    
    ?file a knora-base:StillImageFileValue .
    
} ORDER BY ?file
  1. I downloaded the result as CSV: ordered list of file value IRIs

  2. I did a diff on the two CSV: no differences shown

#Questions:

  • How would someone convert the data from an existing dump if this branch was already merged back to develop? The data would not load because of the consistency checking. Could there be a load script which would not check for consistency just for this case? Then the data could be written to a dump and reloaded with consistency checking.

  • In Knora-ui, there is still some logic that deals with marking preview file values when reading them. This would not be necessary anymore. Could you please check with the Knora-ui team if this logic could be removed in a future version of Knora-ui? I think the current Knora-ui should work fine with this PR, the preview marking logic is just not necessary.

@benjamingeer
Copy link
Author

I did a diff on the two CSV: no differences show

Great, thank you!

How would someone convert the data from an existing dump if this branch was already merged back to develop?

There would be no problem as long as their dump contains the old knora-base, which it would if it's a full backup of the repository.

@tobiasschweizer
Copy link
Contributor

@kilchenmann is on holiday

@benjamingeer
Copy link
Author

@kilchenmann @flavens Tobias says:

In Knora-ui, there is still some logic that deals with marking preview file values when reading them. This would not be necessary anymore. Could you please check with the Knora-ui team if this logic could be removed in a future version of Knora-ui? I think the current Knora-ui should work fine with this PR, the preview marking logic is just not necessary.

@benjamingeer benjamingeer merged commit c069371 into develop Feb 28, 2019
@benjamingeer benjamingeer deleted the wip/1068-remove-previews branch February 28, 2019 09:53
@tobiasschweizer
Copy link
Contributor

@benjamingeer I will redo my Meditationes-import again this afternoon (converted data as basis). If problems occur, I would come back to you. Burt I think it should be fine.

@benjamingeer
Copy link
Author

@tobiasschweizer OK, thanks very much.

@tobiasschweizer
Copy link
Contributor

this is the issue in Knora-ui: dasch-swiss/knora-ui#190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants