Skip to content

Rearrange sessionbuilder and tracing#351

Draft
javiermtorres wants to merge 1 commit intomainfrom
343-ort-session-mgmt
Draft

Rearrange sessionbuilder and tracing#351
javiermtorres wants to merge 1 commit intomainfrom
343-ort-session-mgmt

Conversation

@javiermtorres
Copy link
Copy Markdown
Contributor

This is meant to be a basis for discussion about a preliminary rearrangement of ort session config and auxiliary services like tracing.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 43.24324% with 21 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
encoderfile/src/transport/cli.rs 0.00% 13 Missing ⚠️
encoderfile-runtime/src/main.rs 0.00% 3 Missing ⚠️
encoderfile/src/runtime/state.rs 82.35% 3 Missing ⚠️
encoderfile/src/runtime/loader.rs 0.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
encoderfile/src/builder/model.rs 91.83% <100.00%> (ø)
encoderfile/src/dev_utils/mod.rs 97.95% <100.00%> (ø)
encoderfile/src/runtime/loader.rs 64.00% <0.00%> (ø)
encoderfile-runtime/src/main.rs 0.00% <0.00%> (ø)
encoderfile/src/runtime/state.rs 77.27% <82.35%> (+3.19%) ⬆️
encoderfile/src/transport/cli.rs 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@javiermtorres
Copy link
Copy Markdown
Contributor Author

Ok, tracing will be kept as is. As for the rest:

we should make execution providers available on a per-binary basis.

Ok, so we'd enable flags in build saying something like "--ep=coreml" (or whatever). Then we can choose the specific provider and include it in the ef. This limits a bit the handiness of having a generic binary that can be customized for some specific model merely by changing the footer, but once the user gets one with the support they want, they can reuse it as much as they want.

Each binary that supports an EP should either use that EP or CPU at runtime (specified with a CLI command)

Ok, something like "--cpu"?

Are those n_intra_threads + parallel execution arguments new? They might be cpu-only

They could be. I've added them only as an example of onnx knobs we might be interested in when creating the session. I'm not sure we should expose all, some or any at all 😕

We should name GlobalState something else. Now we have 3 states and they will be easy to confuse 😬

Totally 😂 I didn't know what to name it, nor if you'd like to have some global state as such (we're not currently handling threads but we will). OnnxFactory could be a suggestion, but 1) let's check if we want indeed some global top state, and 2) let's choose a fancy name 😄

If you'd agree, I'll go for the top two first. We could limit the scope of this PR just to them, maybe, and replace the GlobalState with just a function returning the hardcoded SessionBuilder for the moment.

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.

2 participants