fix: migration of old datasets by jsam · Pull Request #639 · SwissDataScienceCenter/renku-python

jsam · 2019-08-23T20:11:13Z

closes #636
closes #646

rokroskar · 2019-08-26T22:14:12Z

can you make sure to check for any paths in a Dataset that are outside the repo root? Those should be migrated, see #646 which will fail with

stderr: 'fatal: ../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv: '../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv' is outside repository

even if the bug which causes the reported problem is fixed...

accidentally approved :oops: #sorry

jsam · 2019-08-29T10:21:03Z

can you make sure to check for any paths in a Dataset that are outside the repo root? Those should be migrated, see #646 which will fail with
stderr: 'fatal: ../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv: '../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv' is outside repository
even if the bug which causes the reported problem is fixed...

I'm trying to reproduce it, but still not fully understanding how that happened. Let's merge this as is until I can look more in-depth into #646 issue.

rokroskar · 2019-08-29T16:04:31Z

Using https://dev.renku.ch/projects/lorenzo.cavazzi.tech/zurich-bikes-tutorial I get:

renku doctor
...
stderr: 'fatal: ../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv: '../../../data/zhbikes/2019_verkehrszaehlungen_werte_fussgaenger_velo.csv' is outside repository

rokroskar

Thanks for looking into this! I think checking for the old-style relative paths that point to outside the repo is the final thing to add.

rokroskar · 2019-08-29T16:37:02Z

Running through the toy example from #636 (comment) but using the code from this PR instead of renku 0.5.2 I end up in this state:

 git status
On branch master
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        deleted:    data/mydata/metadata.yml

and I still have a submodule in the repo:

cat .gitmodules
[submodule "test-migration"]
        path = .renku/vendors/local/private/tmp/test-migration
        url = /private/tmp/test-migration

checked out in the .renku/vendors/local directory:

tree .renku
.renku
├── datasets
│   └── 164bc1a7-3919-4851-8975-230242852f0e
│       └── metadata.yml
├── metadata.yml
├── refs
│   └── datasets
│       └── mydata -> ../../datasets/164bc1a7-3919-4851-8975-230242852f0e/metadata.yml
└── vendors
    └── local
        └── private
            └── tmp
                └── test-migration
                    ├── Dockerfile
                    ├── data
                    │   └── mydata
                    │       └── metadata.yml
                    ├── datafile
                    └── requirements.txt

rokroskar · 2019-08-30T11:34:44Z

argh... I'm finding now another issue where the file paths are absolute - so they can't be resolved when the repo is cloned somewhere else. Do you want to deal with that in the migration as well? Otherwise I need to check for this every time I instantiate a DatasetFile.

jsam · 2019-08-30T11:50:29Z

How did you test this? I tested it again this morning and it works for me and also I have a test which reproduces the report from #636 which is passing.

rokroskar · 2019-09-04T08:27:30Z

~~I don't see any different behavior~~

looks good :D

rokroskar · 2019-09-04T22:33:40Z

Trying to migrate the dataset from https://renkulab.io/gitlab/ACE-ASAID/meteorology (after deleting the new one) I get

renku dataset ls-files
Error: Broken resource of type `datasets` found.
Migration is required.
Please check `renku migrate --help` command to fix the issue.
Hint: `renku migrate datasets`

which I guess is because of absolute paths:

cat .renku/datasets/ce2e0f25-dff1-49f5-8d37-19aaa12c0e22/metadata.yml
'@context':
  _id: '@id'
  _label: rdfs:label
  _project: schema:isPartOf
  created: schema:dateCreated
  creator: schema:creator
  date_published: schema:datePublished
  description: schema:description
  files: schema:hasPart
  identifier: schema:identifier
  in_language: schema:inLanguage
  keywords: schema:keywords
  license: schema:license
  name: schema:name
  path: prov:atLocation
  prov: http://www.w3.org/ns/prov#
  schema: http://schema.org/
  url: schema:url
  version: schema:version
  wfprov: http://purl.org/wf4ever/wfprov#
'@type':
- prov:Entity
- schema:Dataset
- wfprov:Artifact
_id: ce2e0f25-dff1-49f5-8d37-19aaa12c0e22
_label: ce2e0f25-dff1-49f5-8d37-19aaa12c0e22
_project:
  '@context':
    _id: '@id'
    foaf: http://xmlns.com/foaf/0.1/
    prov: http://www.w3.org/ns/prov#
  '@type':
  - foaf:Project
  - prov:Location
  _id: https://renkulab.io/ACE-ASAID/meteorology
authors:
- '@type': dcterms:creator
  affiliation: null
  email: jenny_t152@yahoo.co.uk
  name: Jen Thomas
created: '2018-11-23T14:42:17.985570'
creator: []
date_published: null
description: null
files:
- '@type':
  - prov:Entity
  - schema:DigitalDocument
  - wfprov:Artifact
  _id: blob/UNCOMMITTED/MAWS__SMSAWS__20161123.txt
  _label: MAWS__SMSAWS__20161123.txt@UNCOMMITTED
  _project: null
  added: '2018-11-23T15:12:42.232297'
  creator: []
  dataset: null
  name: MAWS__SMSAWS__20161123.txt
  path: MAWS__SMSAWS__20161123.txt
  url: /home/jen/projects/ace_data_management/data_staging_for_renku/meteorology/MAWS__SMSAWS__20161123.txt
- '@type':
  - prov:Entity
  - schema:DigitalDocument
  - wfprov:Artifact
  _id: blob/UNCOMMITTED/MAWS__SMSAWS__20161124.txt
  _label: MAWS__SMSAWS__20161124.txt@UNCOMMITTED
  _project: null
  added: '2018-11-23T15:12:42.257562'
  creator: []
  dataset: null
  name: MAWS__SMSAWS__20161124.txt
  path: MAWS__SMSAWS__20161124.txt
  url: /home/jen/projects/ace_data_management/data_staging_for_renku/meteorology/MAWS__SMSAWS__20161124.txt
...

rokroskar

Looking more closely at the migration code, there is a lot of looping through the datasets and files - this could get annoyingly expensive. Wouldn't it be better to loop once and define the necessary functions either at the Dataset or DatasetFile level?

I also have some issues with the tests, getting output like:

fatal: repository '/home/sam/Work/data_repos/old-datasets-v0.3.0' does not exist
fatal: clone of '/home/sam/Work/data_repos/old-datasets-v0.3.0' into submodule path '/private/var/folders/99/jng0jljd13db0h4jwbwtx4mh0000gn/T/pytest-of-rok/pytest-18/repo0/.renku/vendors/local/home/sam/Work/data_repos/old-datasets-v0.3.0' failed
Failed to clone '.renku/vendors/local/home/sam/Work/data_repos/old-datasets-v0.3.0'. Retry scheduled
fatal: repository '/home/sam/Work/data_repos/old-datasets-v0.3.0' does not exist
fatal: clone of '/home/sam/Work/data_repos/old-datasets-v0.3.0' into submodule path '/private/var/folders/99/jng0jljd13db0h4jwbwtx4mh0000gn/T/pytest-of-rok/pytest-18/repo0/.renku/vendors/local/home/sam/Work/data_repos/old-datasets-v0.3.0' failed
Failed to clone '.renku/vendors/local/home/sam/Work/data_repos/old-datasets-v0.3.0' a second time, aborting

Structurally, I don't understand why the migrations (or the tests) are lumped under "CLI"? In both contexts it should be under "models" imho.

rokroskar · 2019-09-07T21:43:42Z

Another thing I noticed - authors is no longer migrated to creator - it looks like the model migrations are now skipped?

rokroskar · 2019-09-07T22:00:23Z

apologies, my repo found itself in a state from which the only easy way out was a force-push...

rokroskar

almost there - I think the lock file handling is the last small detail. Otherwise all the other problems seem to be fixed. Thanks!!

rokroskar · 2019-09-12T22:17:18Z

+        )
+
+
+class NothingToCommit(RenkuException, click.ClickException):


I wonder if this should be an error or a warning?

IMHO, empty commits should never happen from renku side. Now I agree that it's not the best UX, but currently, due to how things are implemented (via decorators) we don't have an effective way of capturing this exception and showing nice message to the user. 😞

rokroskar

I'm not entirely sure about returning an error when there is nothing to commit, but otherwise it all seems to work now!! 🎉

jsam requested a review from a team as a code owner August 23, 2019 20:11

rokroskar mentioned this pull request Aug 23, 2019

fix: use correct paths during migration #637

Closed

4 tasks

jsam changed the title ~~fix: migration of old datasets~~ [WIP] fix: migration of old datasets Aug 23, 2019

rokroskar requested changes Aug 27, 2019

View reviewed changes

Comment thread renku/cli/_providers/zenodo.py Outdated

Comment thread renku/cli/_providers/zenodo.py Outdated

Comment thread renku/cli/migrate.py Outdated

rokroskar mentioned this pull request Aug 27, 2019

fix: always add commit to dataset if possible #648

Merged

1 task

rokroskar previously approved these changes Aug 27, 2019

View reviewed changes

jsam changed the title ~~[WIP] fix: migration of old datasets~~ fix: migration of old datasets Aug 29, 2019

rokroskar requested changes Aug 29, 2019

View reviewed changes

Comment thread renku/api/repository.py

Comment thread renku/api/repository.py

Comment thread renku/api/repository.py

Comment thread renku/cli/_checks/migrate_datasets.py

Comment thread renku/cli/migrate.py Outdated

jsam requested a review from rokroskar September 3, 2019 08:07

rokroskar requested changes Sep 7, 2019

View reviewed changes

rokroskar requested changes Sep 10, 2019

View reviewed changes

Comment thread renku/cli/_checks/migrate_datasets.py

Comment thread renku/models/migrations/__init__.py

rokroskar reviewed Sep 10, 2019

View reviewed changes

Comment thread renku/cli/_checks/migrate_datasets.py

rokroskar reviewed Sep 11, 2019

View reviewed changes

Comment thread tests/cli/test_migrate.py

rokroskar requested changes Sep 11, 2019

View reviewed changes

Comment thread renku/cli/_checks/migrate_datasets.py Outdated

Comment thread renku/cli/_checks/migrate_datasets.py Outdated

Comment thread renku/cli/_checks/migrate_datasets.py Outdated

Comment thread renku/models/migrations/__init__.py

Comment thread renku/cli/_checks/migrate_datasets.py

jsam requested a review from rokroskar September 12, 2019 13:15

fix: migration of old datasets

8489b92

rokroskar reviewed Sep 12, 2019

View reviewed changes

rokroskar approved these changes Sep 12, 2019

View reviewed changes

jsam merged commit 4d4d7d2 into SwissDataScienceCenter:master Sep 12, 2019

jsam deleted the 636-fix-migration-00 branch September 12, 2019 22:27

		)


		class NothingToCommit(RenkuException, click.ClickException):

Conversation

jsam commented Aug 23, 2019 • edited by rokroskar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rokroskar commented Aug 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsam commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rokroskar commented Aug 29, 2019

Uh oh!

rokroskar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rokroskar commented Aug 29, 2019

Uh oh!

rokroskar commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsam commented Aug 30, 2019

Uh oh!

rokroskar commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rokroskar commented Sep 4, 2019

Uh oh!

rokroskar left a comment

Choose a reason for hiding this comment

Uh oh!

rokroskar commented Sep 7, 2019

Uh oh!

rokroskar commented Sep 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rokroskar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rokroskar Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

jsam Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

rokroskar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsam commented Aug 23, 2019 •

edited by rokroskar

Loading

jsam commented Aug 29, 2019 •

edited

Loading

rokroskar commented Aug 30, 2019 •

edited

Loading

rokroskar commented Sep 4, 2019 •

edited

Loading