Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Raw output prefix #169

Merged
merged 16 commits into from
Aug 12, 2020
Merged

Raw output prefix #169

merged 16 commits into from
Aug 12, 2020

Conversation

wild-endeavor
Copy link
Contributor

Read then delete

  • Make sure to use a concise title for the pull-request.
  • Use #patch, #minor or #major in the pull-request title to bump the corresponding version. Otherwise, the patch version
    will be bumped. More details

TL;DR

Please replace this text with a description of what this PR accomplishes.

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

flyteorg/flyte#211

Follow-up issue

NA

Yee Hing Tong added 2 commits July 31, 2020 11:05
c.metrics.InterruptedThresholdHit.Inc(ctx)
}

rawOutputPrefix := c.defaultDataSandbox
if executionContext.GetRawOutputDataConfig().RawOutputDataConfig != nil && executionContext.GetRawOutputDataConfig().OutputLocationPrefix != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm why the 2 checks?
So node execution context is recursive, so for a node, it should always be parent node exec context + some stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. But this meant I had to fix a bunch of unit tests.

@kumare3
Copy link
Contributor

kumare3 commented Jul 31, 2020

Mostly looks good to me, let me just dive deeper into the actual node context creation part

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2020

Codecov Report

Merging #169 into master will decrease coverage by 0.00%.
The diff coverage is 66.66%.

EngHabu
EngHabu previously approved these changes Aug 12, 2020
@EngHabu EngHabu merged commit a2d8a74 into master Aug 12, 2020
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* attempt to bring in IDL change, and add to workflow struct

* go sum

* changes

* spelling

* make generate

* just use meta

* unit tests

* wip

* wip

* make goimports

* wip

* add comment in config.yaml

* Revert local changes

Co-authored-by: Haytham AbuelFutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants