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

Adopt flyteidl's ordered variable map change #608

Merged
merged 23 commits into from
Aug 31, 2021

Conversation

mayitbeegh
Copy link
Contributor

@mayitbeegh mayitbeegh commented Aug 23, 2021

Signed-off-by: Sean Lin [email protected]

TL;DR

See flyteorg/flyteidl#206

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/lyft/flyte/issues/

Follow-up issue

NA
OR
https://github.com/lyft/flyte/issues/

Copy link
Collaborator

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Shouldn't there be an update in setup.py for the version?

Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

there's a couple failed tests too i think... and by a couple i mean 18,238 :)

return _interface_pb2.VariableMap(variables={k: v.to_flyte_idl() for k, v in _six.iteritems(self.variables)})
return _interface_pb2.VariableMap(
variables=[
_interface_pb2.VariableMapEntry(name=k, var=v.to_flyte_idl()) for k, v in _six.iteritems(self.variables)
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're at it, let's delete six from this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a couple failed tests too i think... and by a couple i mean 18,238 :)

It worked on my machine...
I did pip install -e for the flyteidl dependency. I'll update the version once the flyteidl change is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while we're at it, let's delete six from this file.

And drop Python 2 compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

we haven't had python 2 compatibility in forever... it's just too much work to delete six all at once so i generally just do it when i'm working in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

@mayitbeegh
Copy link
Contributor Author

Shouldn't there be an update in setup.py for the version?

I did pip install -e for the flyteidl dependency. I'll update the version once the flyteidl change is released. Let me know if there's a better way to do this.

* remove six
* change parameter field name

Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Sean Lin <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review August 27, 2021 17:00
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

lgtm, can approve once the idl change is merged in

Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Sean Lin <[email protected]>
@mayitbeegh mayitbeegh changed the title wip Adopt flyteidl's ordered variable map change Adopt flyteidl's ordered variable map change Aug 30, 2021
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #608 (d853840) into master (44c74b9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head d853840 differs from pull request most recent head 4b14330. Consider uploading reports for the commit 4b14330 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #608      +/-   ##
==========================================
+ Coverage   85.85%   85.86%   +0.01%     
==========================================
  Files         397      397              
  Lines       31150    31112      -38     
  Branches     2502     2495       -7     
==========================================
- Hits        26743    26714      -29     
+ Misses       3732     3723       -9     
  Partials      675      675              
Impacted Files Coverage Δ
flytekit/models/interface.py 98.75% <100.00%> (ø)
...sts/flytekit/unit/common_tests/test_launch_plan.py 100.00% <100.00%> (ø)
tests/flytekit/unit/common_tests/test_workflow.py 96.32% <100.00%> (+0.06%) ⬆️
tests/flytekit/unit/remote/test_remote.py 100.00% <0.00%> (ø)
flytekit/remote/remote.py 74.17% <0.00%> (+0.19%) ⬆️
flytekit/clients/friendly.py 68.00% <0.00%> (+0.25%) ⬆️
flytekit/remote/tasks/task.py 94.59% <0.00%> (+4.35%) ⬆️

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 44c74b9...4b14330. Read the comment docs.

wild-endeavor and others added 8 commits August 31, 2021 10:54
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
This reverts commit 677bdb9.

Signed-off-by: Eduardo Apolinario <[email protected]>
@wild-endeavor wild-endeavor merged commit 204b13f into master Aug 31, 2021
mayitbeegh added a commit that referenced this pull request Sep 9, 2021
mayitbeegh added a commit that referenced this pull request Sep 9, 2021
mayitbeegh added a commit that referenced this pull request Sep 10, 2021
This reverts commit 204b13f
Upgrade flyteidl
Signed-off-by: Sean Lin <[email protected]>
mayitbeegh added a commit that referenced this pull request Sep 10, 2021
This reverts commit 204b13f
Upgrade flyteidl
Signed-off-by: Sean Lin <[email protected]>
mayitbeegh added a commit that referenced this pull request Sep 10, 2021
* Revert "Adopt flyteidl's ordered variable map change (#608)"
This reverts commit 204b13f
* Upgrade flyteidl to 0.21.0
Signed-off-by: Sean Lin <[email protected]>
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.

5 participants