Update core package #653

mrchtr · 2023-11-20T14:55:08Z

First PR related to the data structure redesign.

Implements the following:

New manifest structure (including validation, and evolution)
New ComponentSpec structure (including validation)
Removes Subsets and Index

Not all tests are running successfully. But this are already quite a few changes. Therefore, I've created PR on feature branch feature/redesign-dataset-format-and-interface, to have quicker feedback loops.

RobbeSneyders

Thanks @mrchtr! I did a first quick review.

I would focus on getting the core module ported first and make sure the tests for those are passing. So everything related to the component spec and manifest.

Could you remove all the other changes from this branch and open those as PRs to this one? That would make it a lot easier to review :)

src/fondant/core/component_spec.py

src/fondant/core/manifest.py

src/fondant/core/schema.py

src/fondant/core/schemas/common.json

src/fondant/core/schemas/manifest.json

src/fondant/core/component_spec.py

RobbeSneyders

Thanks @mrchtr, clean work!

I still need to review the tests. I would split the test examples per module as well though.

src/fondant/core/component_spec.py

src/fondant/core/manifest.py

src/fondant/core/schema.py

src/fondant/core/schemas/component_spec.json

Co-authored-by: Robbe Sneyders <[email protected]>

mrchtr · 2023-11-22T07:25:31Z

I still need to review the tests. I would split the test examples per module as well though.

I've cleaned the test a bit more. I moved the relevant sample files into tests/core/examples.
Removed some of the test cases for manifest evolution. Mainly the ones related to additionalFields. We have to keep this in mind when we are implementing #656. We should add new test (re-add relevant) cases after the implementation.

src/fondant/core/manifest.py

src/fondant/core/schema.py

tests/core/examples/evolution_examples/1/output_manifest.json

PhilippeMoussalli · 2023-11-22T10:40:16Z

src/fondant/core/manifest.py

+        else:
+            self._specification["fields"][field.name] = {
+                "location": f"/{self.component_id}",
+                "type": field.type.name,


See my comment below. We should store them as json mainly to match the format of the component spec

Suggested change

"type": field.type.name,

"type": field.type.to_json(),

Thanks! Wasn't aware of this, but it makes totally sense!

This broke some of the tests though.

>>> type.to_json() { "type": type }

So now we get:

{ "location": ..., "type": { "type": type } }

I think we can solve it like this:

self._specification["fields"][field.name] = { "location": f"/{self.component_id}", **field.type.name, }

src/fondant/core/component_spec.py

src/fondant/core/manifest.py

Co-authored-by: Philippe Moussalli <[email protected]>

RobbeSneyders

Thanks @mrchtr, I think these are the last ones. I think the tests should pass with these changes.

src/fondant/core/manifest.py

tests/core/test_component_specs.py

tests/core/test_manifest_evolution.py

RobbeSneyders · 2023-11-22T20:23:03Z

tests/examples/example_specs/components/component.yaml

+    type: array
+    items:
+      type: float32
+additionalFields: false


This was removed for now, right?

Would leave it here for now. The file is moved to a different folder anyway in #655.

tests/core/examples/evolution_examples/1/output_manifest.json

RobbeSneyders · 2023-11-22T20:40:27Z

src/fondant/core/manifest.py

+        else:
+            self._specification["fields"][field.name] = {
+                "location": f"/{self.component_id}",
+                "type": field.type.name,


This broke some of the tests though.

>>> type.to_json() { "type": type }

So now we get:

{ "location": ..., "type": { "type": type } }

I think we can solve it like this:

self._specification["fields"][field.name] = { "location": f"/{self.component_id}", **field.type.name, }

RobbeSneyders · 2023-11-22T21:54:16Z

FYI, as merge strategy, let's squash merge your separate PRs into your feature branch, and then rebase and merge the feature branch when it's ready. Then we keep the PRs as separate commits on main.

RobbeSneyders

Great work @mrchtr!

First PR related to the data structure redesign. Implements the following: - New manifest structure (including validation, and evolution) - New ComponentSpec structure (including validation) - Removes `Subsets` and `Index` Not all tests are running successfully. But this are already quite a few changes. Therefore, I've created PR on feature branch `feature/redesign-dataset-format-and-interface`, to have quicker feedback loops. --------- Co-authored-by: Robbe Sneyders <[email protected]> Co-authored-by: Philippe Moussalli <[email protected]>

mrchtr added 5 commits November 16, 2023 14:15

Update component spec schema validation

35338b6

Update component spec tests to validate new component spec

a269e3c

Add additional fields to json schema

ad0dab6

Update manifest json schema for validation

7b91535

Update manifest creation

5d1bf5e

RobbeSneyders reviewed Nov 20, 2023

View reviewed changes

Reduce PR to core module

d8ecd01

mrchtr force-pushed the feature/implement-new-dataset-format branch from 6bb9aa0 to d8ecd01 Compare November 21, 2023 08:26

mrchtr added 4 commits November 21, 2023 10:39

Addresses comments

12c78ca

Restructure test directory

c1cad60

Remove additional fields in common.json

fd0699c

Test structure

0f8117f

RobbeSneyders reviewed Nov 21, 2023

View reviewed changes

mrchtr and others added 9 commits November 21, 2023 14:57

Update src/fondant/core/component_spec.py

9f67c61

Co-authored-by: Robbe Sneyders <[email protected]>

Update src/fondant/core/manifest.py

40955bf

Co-authored-by: Robbe Sneyders <[email protected]>

Update src/fondant/core/component_spec.py

6b246a4

Co-authored-by: Robbe Sneyders <[email protected]>

Update src/fondant/core/manifest.py

8ef38d9

Co-authored-by: Robbe Sneyders <[email protected]>

Update src/fondant/core/schema.py

e8c8135

Co-authored-by: Robbe Sneyders <[email protected]>

Addresses comments

df9a60e

Addresses comments

2256118

Addresses comments

3042fb5

Update src/fondant/core/manifest.py

8fa8be7

Co-authored-by: Robbe Sneyders <[email protected]>

mrchtr changed the title ~~Implement new data structure, ComponentSpec and Manifest~~ Update component package Nov 22, 2023

mrchtr linked an issue Nov 22, 2023 that may be closed by this pull request

Update core package #652

Closed

mrchtr changed the title ~~Update component package~~ Update core package Nov 22, 2023

Addresses comments

25eb492

mrchtr mentioned this pull request Nov 22, 2023

Implement previous_index #656

Closed

PhilippeMoussalli reviewed Nov 22, 2023

View reviewed changes

src/fondant/core/manifest.py Outdated Show resolved Hide resolved

PhilippeMoussalli reviewed Nov 22, 2023

View reviewed changes

src/fondant/core/schema.py Outdated Show resolved Hide resolved

PhilippeMoussalli reviewed Nov 22, 2023

View reviewed changes

tests/core/examples/evolution_examples/1/output_manifest.json Outdated Show resolved Hide resolved

PhilippeMoussalli reviewed Nov 22, 2023

View reviewed changes

RobbeSneyders reviewed Nov 22, 2023

View reviewed changes

src/fondant/core/component_spec.py Outdated Show resolved Hide resolved

src/fondant/core/component_spec.py Outdated Show resolved Hide resolved

src/fondant/core/manifest.py Outdated Show resolved Hide resolved

src/fondant/core/manifest.py Outdated Show resolved Hide resolved

mrchtr and others added 2 commits November 22, 2023 13:23

Update src/fondant/core/manifest.py

4dc7dc7

Co-authored-by: Philippe Moussalli <[email protected]>

addresses comments

a60ca3e

RobbeSneyders reviewed Nov 22, 2023

View reviewed changes

Addresses comments

4112fb0

RobbeSneyders approved these changes Nov 23, 2023

View reviewed changes

RobbeSneyders merged commit 06186c1 into feature/redesign-dataset-format-and-interface Nov 23, 2023
1 of 5 checks passed

RobbeSneyders deleted the feature/implement-new-dataset-format branch November 23, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core package #653

Update core package #653

mrchtr commented Nov 20, 2023 •

edited

Loading

RobbeSneyders left a comment

RobbeSneyders left a comment •

edited

Loading

mrchtr commented Nov 22, 2023

PhilippeMoussalli Nov 22, 2023

mrchtr Nov 22, 2023

RobbeSneyders Nov 22, 2023

RobbeSneyders left a comment

RobbeSneyders Nov 22, 2023

mrchtr Nov 23, 2023

RobbeSneyders Nov 22, 2023

RobbeSneyders commented Nov 22, 2023

RobbeSneyders left a comment

Update core package #653

Update core package #653

Conversation

mrchtr commented Nov 20, 2023 • edited Loading

RobbeSneyders left a comment

Choose a reason for hiding this comment

RobbeSneyders left a comment • edited Loading

Choose a reason for hiding this comment

mrchtr commented Nov 22, 2023

PhilippeMoussalli Nov 22, 2023

Choose a reason for hiding this comment

mrchtr Nov 22, 2023

Choose a reason for hiding this comment

RobbeSneyders Nov 22, 2023

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

RobbeSneyders Nov 22, 2023

Choose a reason for hiding this comment

mrchtr Nov 23, 2023

Choose a reason for hiding this comment

RobbeSneyders Nov 22, 2023

Choose a reason for hiding this comment

RobbeSneyders commented Nov 22, 2023

RobbeSneyders left a comment

Choose a reason for hiding this comment

mrchtr commented Nov 20, 2023 •

edited

Loading

RobbeSneyders left a comment •

edited

Loading