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

Add GetSpanPath and SpanExists #361

Conversation

Yicheng-Lu-llll
Copy link
Member

@Yicheng-Lu-llll Yicheng-Lu-llll commented Jun 14, 2023

TL;DR

This is part of Flytekit Metrics Exploration.

This PR helps propeller to get span uri(highlighted in green).

image

Flytekit Metrics Exploration includes:

Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
@welcome
Copy link

welcome bot commented Jun 14, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
@Yicheng-Lu-llll Yicheng-Lu-llll changed the title [WIP] Add GetSpanPath Add GetSpanPath and SpanExists Jun 14, 2023
@Yicheng-Lu-llll Yicheng-Lu-llll marked this pull request as ready for review June 14, 2023 04:26
Signed-off-by: Yicheng-Lu-llll <[email protected]>
return r.SpanPath != nil, nil
}

func NewInMemoryOutputReader(literals *core.LiteralMap, DeckPath *storage.DataReference, SpanPath *storage.DataReference, err *io.ExecutionError) InMemoryOutputReader {
Copy link
Member

@pingsutw pingsutw Jun 16, 2023

Choose a reason for hiding this comment

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

Could we add a new function called NewInMemoryOutputReaderWithSpan? Spotify is using this function internally, so we can't change it. #268 (comment)

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #361 (cb694b8) into master (2a7a6f9) will increase coverage by 1.38%.
The diff coverage is 84.84%.

❗ Current head cb694b8 differs from pull request most recent head a70022d. Consider uploading reports for the commit a70022d to get more accurate results

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   62.62%   64.00%   +1.38%     
==========================================
  Files         152      152              
  Lines       12789    10388    -2401     
==========================================
- Hits         8009     6649    -1360     
+ Misses       4168     3126    -1042     
- Partials      612      613       +1     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
go/tasks/pluginmachinery/ioutils/paths.go 84.61% <ø> (+4.61%) ⬆️
go/tasks/plugins/array/awsbatch/monitor.go 68.31% <0.00%> (+0.89%) ⬆️
go/tasks/plugins/array/k8s/management.go 57.21% <0.00%> (-1.12%) ⬇️
go/tasks/plugins/k8s/sagemaker/builtin_training.go 72.00% <0.00%> (+4.99%) ⬆️
...sks/plugins/k8s/sagemaker/hyperparameter_tuning.go 60.45% <0.00%> (+5.24%) ⬆️
go/tasks/plugins/presto/execution_state.go 50.65% <0.00%> (+0.65%) ⬆️
...pluginmachinery/ioutils/in_memory_output_reader.go 63.63% <50.00%> (-4.55%) ⬇️
go/tasks/plugins/array/outputs.go 73.29% <50.00%> (+2.73%) ⬆️
...uginmachinery/ioutils/remote_file_output_reader.go 22.50% <60.00%> (+2.50%) ⬆️
go/tasks/logs/logging_utils.go 90.00% <100.00%> (+2.50%) ⬆️
... and 7 more

... and 116 files with indirect coverage changes

Signed-off-by: Yicheng-Lu-llll <[email protected]>
@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4128. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
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.

3 participants