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

Modified the condition to load the optimiser model #18891

Conversation

heflinstephenraj
Copy link
Contributor

Description

I modified the condition in the if-clause to set the value for optimizerStr.

Motivation and Context

Since the condition is optimizerPath == NULL, the program control flow doesn't enter the if block. As a result, the value for optimizerStr is not set, and consequently, the optimizer model is not loaded. I have made the necessary changes to set the value for optimizerStr.

Since the condition is optimizerPath == NULL, the program control flow doesn't enter the if block. As a result, the value for optimizerStr is not set, and consequently, the optimizer model is not loaded. I have made the necessary changes to set the value for optimizerStr.
@Craigacp
Copy link
Contributor

LGTM. @baijumeswani I would have expected this typo to be caught by this test, is there no pipeline which runs training + Java + Windows?

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@baijumeswani
Copy link
Contributor

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@heflinstephenraj
Copy link
Contributor Author

Hi,
Is there anything that needs to be done on my side to merge this pull request?
@baijumeswani @Craigacp

@baijumeswani baijumeswani merged commit 0ea48fc into microsoft:main Jan 23, 2024
72 of 80 checks passed
@baijumeswani
Copy link
Contributor

heflinstephenraj sorry I missed approving and merging. Thanks for the reminder and thank you for your contribution.

@Craigacp
Copy link
Contributor

Is it possible to get this into 1.17? I realise the RC has come out, but this is a very small fix.

@baijumeswani
Copy link
Contributor

Yes, I tagged it to be cherry picked for 1.17 and will coordinate with the release manager to have this be a part of ort 1.17 release.

@Craigacp
Copy link
Contributor

Awesome, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants