Skip to content

[pr into #785] Remove fsspec#811

Merged
wild-endeavor merged 4 commits into
structured-dataset-proposalfrom
rm-fsspec
Jan 12, 2022
Merged

[pr into #785] Remove fsspec#811
wild-endeavor merged 4 commits into
structured-dataset-proposalfrom
rm-fsspec

Conversation

@pingsutw

Copy link
Copy Markdown
Member

TL;DR

  • Remove fsspec
  • remote util.py
  • Add e2e test to test_workflows.py
  • Revert data_persistence.py

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@codecov

codecov Bot commented Jan 11, 2022

Copy link
Copy Markdown

Codecov Report

Merging #811 (b3270a4) into structured-dataset-proposal (e66f80f) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           structured-dataset-proposal     #811      +/-   ##
===============================================================
+ Coverage                        85.71%   85.88%   +0.16%     
===============================================================
  Files                              356      356              
  Lines                            30585    30727     +142     
  Branches                          3679     3680       +1     
===============================================================
+ Hits                             26216    26389     +173     
+ Misses                            3700     3672      -28     
+ Partials                           669      666       -3     
Impacted Files Coverage Δ
flytekit/core/data_persistence.py 69.79% <ø> (-0.32%) ⬇️
flytekit/types/structured/basic_dfs.py 100.00% <100.00%> (+7.27%) ⬆️
tests/flytekit/unit/core/test_flyte_file.py 98.52% <100.00%> (+0.01%) ⬆️
tests/flytekit/unit/core/test_workflows.py 99.01% <100.00%> (+0.22%) ⬆️
...ctured_dataset/test_structured_dataset_workflow.py 100.00% <100.00%> (ø)
flytekit/core/map_task.py 81.63% <0.00%> (ø)
tests/flytekit/unit/core/test_map_task.py 94.00% <0.00%> (ø)
flytekit/types/structured/structured_dataset.py 84.28% <0.00%> (+3.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e66f80f...b3270a4. Read the comment docs.

Comment on lines +18 to +21
try:
from typing import Annotated
except ImportError:
from typing_extensions import Annotated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to do this? We're already pulling typing_extensions as part of a regular flytekit installation, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm? no we need this. this is for compatibility between 3.7/8 and 3.9

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why? We already have typing_extensions, so we could rely on that for all versions, including 3.9-10.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From the user side, they may use from typing import Annotated or from typing_extensions import Annotated. Therefore, we need to make sure both of them can work here.

Comment thread tests/flytekit/unit/core/test_workflows.py Outdated
df = structured_dataset.dataframe
pq.write_table(df, path, filesystem=get_filesystem(path))
local_dir = ctx.file_access.get_random_local_directory()
local_path = os.path.join(local_dir, f"{0:05}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you leave a comment explaining why we need a file named 00000? This is already present in dataset_example.py (I'm not sure why), but that doesn't have a comment either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will add a comment... this is only to keep the semantics the same as existing behavior.

@wild-endeavor

Copy link
Copy Markdown
Contributor

i think one of the integration tests timed out too, next run will probably be fine.

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@wild-endeavor wild-endeavor merged commit f9358c8 into structured-dataset-proposal Jan 12, 2022
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.

3 participants