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

Change field order to drop Environment last #105

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented Oct 23, 2023

This changes the order of the field to drop Environment after everything else.

The current code gets stuck, randomly but steadily, during the drop. What happened is that SessionPointerHolder held by the session is dropped after the Environment, and this seems to cause it to be blocked indefinitely.

This is an example code that shows that the program gets stuck. This happens with both ort 1.15 and 1.16. It happens with different models.

use ort::{
    environment::Environment,
    session::{Session, SessionBuilder},
};

pub struct Model {
    _session: Session,
}

impl Model {
    fn new() -> Self {
        let environment = Environment::builder().build().unwrap().into_arc();
        let session = SessionBuilder::new(&environment)
            .unwrap()
            .with_model_from_file("./model.onnx")
            .unwrap();

        Model {
            _session: session,
        }
    }
}

fn main() {
    Model::new();
}

With the proposed fix, it doesn't get stuck anymore. This could also be achieved by adding an env field after session in Model, but since Session already holds an Environment, it shouldn't be needed.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6a1c19d) 51.88% compared to head (4c9e861) 52.30%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   51.88%   52.30%   +0.42%     
==========================================
  Files          13       13              
  Lines        1089     1084       -5     
==========================================
+ Hits          565      567       +2     
+ Misses        524      517       -7     
Files Coverage Δ
src/session.rs 64.43% <ø> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@decahedron1 decahedron1 merged commit 4ab5785 into pykeio:main Oct 23, 2023
8 checks passed
@acrrd acrrd deleted the drop_env_last branch October 24, 2023 07:41
@decahedron1 decahedron1 mentioned this pull request Oct 27, 2023
Merged
8 tasks
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