Skip to content

fix add-running-sampler-examples-instructions - #19

Open
Cedrick Mwamba (CedrickMwamba) wants to merge 7 commits into
Qiskit:mainfrom
CedrickMwamba:bugfix/add-running-sampler-instructions
Open

Cedrick Mwamba (CedrickMwamba) wants to merge 7 commits into
Qiskit:mainfrom
CedrickMwamba:bugfix/add-running-sampler-instructions

Conversation

@CedrickMwamba

@CedrickMwamba Cedrick Mwamba (CedrickMwamba) commented Mar 16, 2026 •

Copy link
Copy Markdown

Update the readme with instructions of what to do before running the sampler examples.
This will eliminate the error message, because the configuration file is required. Must be added in the your home directory with account information.

Implements proper error handling for configuration loading:

  • Environment variables (QISKIT_IBM_TOKEN, QISKIT_IBM_INSTANCE) now take priority over JSON config
  • Added ConfigError enum with detailed, actionable error messages
  • Added qkrt_get_last_error() to expose error messages to C code
  • Updated check_result! macro to store error messages
  • Added specific exit codes (401-406) for different configuration errors
  • Updated test examples to display full error details
  • Add a new sample test test_error_handling_run which verify the current status of the configuration

Fixes #128

@CLAassistant

CLAassistant commented Mar 16, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CedrickMwamba Cedrick Mwamba (CedrickMwamba) changed the title fix add-running-sampler-examples-instructions #128 fix add-running-sampler-examples-instructions Mar 16, 2026
@CedrickMwamba Cedrick Mwamba (CedrickMwamba) changed the title fix add-running-sampler-examples-instructions fix#128 add-running-sampler-examples-instructions Mar 16, 2026
@CedrickMwamba Cedrick Mwamba (CedrickMwamba) changed the title fix#128 add-running-sampler-examples-instructions fix #128 add-running-sampler-examples-instructions Mar 16, 2026
@CedrickMwamba Cedrick Mwamba (CedrickMwamba) changed the title fix #128 add-running-sampler-examples-instructions fix add-running-sampler-examples-instructions Mar 16, 2026
@CedrickMwamba
Cedrick Mwamba (CedrickMwamba) marked this pull request as ready for review March 18, 2026 08:45
@CedrickMwamba
Cedrick Mwamba (CedrickMwamba) marked this pull request as draft March 18, 2026 16:58
@aaryav-3

Copy link
Copy Markdown

Hey Cedrick Mwamba (@CedrickMwamba) Thanks so much for jumping on this and putting together a PR, appreciate the effort

After digging into it a little, I think the fix needs to go in a different direction. Here's the thing:

The user in the issue actually did set the environment variables (QISKIT_IBM_TOKEN and QISKIT_IBM_INSTANCE) exactly as the README says, so this isn't really a docs gap. The real problem is that the Rust code in crates/client/src/service.rs goes straight to reading the JSON config file and crashes if it doesn't exist, without ever checking the env vars, which should be expected behaviour, according to existing docs.

So I'm assuming the fix would actually need to be a code change

  1. Check env vars first (QISKIT_IBM_TOKEN, QISKIT_IBM_INSTANCE)
  2. Fall back to the JSON file if they're not set
  3. Return a friendly error message instead of panicking if neither is found, by changing the return type to a PyErr (a panic is unwanted, we should always aim for programs to exit gracefully with clear error messages for all public, user APIs)

Also worth noting, documenting the JSON file approach as the main path asks users to store their API token on disk, which isn't ideal from a security standpoint compared to env vars, and well, is also non-ergonomic.

I would suggest inspecting service.rs, handling the panic first (so it's also clear when you reproduce the error, if the code within this is the culprit), if yes, proceed with handling environment variables.

Let me know if you face any issues doing any of these steps, you can read up on
https://doc.rust-lang.org/book/ch09-00-error-handling.html
https://pyo3.rs/main/doc/pyo3/struct.pyerr
https://doc.rust-lang.org/book/ch12-05-working-with-environment-variables.html

To understand syntax and strategy to do this. Do let me know if you face any bottlenecks

1 similar comment
@aaryav-3

Copy link
Copy Markdown

Hey Cedrick Mwamba (@CedrickMwamba) Thanks so much for jumping on this and putting together a PR, appreciate the effort

After digging into it a little, I think the fix needs to go in a different direction. Here's the thing:

The user in the issue actually did set the environment variables (QISKIT_IBM_TOKEN and QISKIT_IBM_INSTANCE) exactly as the README says, so this isn't really a docs gap. The real problem is that the Rust code in crates/client/src/service.rs goes straight to reading the JSON config file and crashes if it doesn't exist, without ever checking the env vars, which should be expected behaviour, according to existing docs.

So I'm assuming the fix would actually need to be a code change

  1. Check env vars first (QISKIT_IBM_TOKEN, QISKIT_IBM_INSTANCE)
  2. Fall back to the JSON file if they're not set
  3. Return a friendly error message instead of panicking if neither is found, by changing the return type to a PyErr (a panic is unwanted, we should always aim for programs to exit gracefully with clear error messages for all public, user APIs)

Also worth noting, documenting the JSON file approach as the main path asks users to store their API token on disk, which isn't ideal from a security standpoint compared to env vars, and well, is also non-ergonomic.

I would suggest inspecting service.rs, handling the panic first (so it's also clear when you reproduce the error, if the code within this is the culprit), if yes, proceed with handling environment variables.

Let me know if you face any issues doing any of these steps, you can read up on
https://doc.rust-lang.org/book/ch09-00-error-handling.html
https://pyo3.rs/main/doc/pyo3/struct.pyerr
https://doc.rust-lang.org/book/ch12-05-working-with-environment-variables.html

To understand syntax and strategy to do this. Do let me know if you face any bottlenecks

@CedrickMwamba

Copy link
Copy Markdown
Author

Hey Cedrick Mwamba (@CedrickMwamba) Thanks so much for jumping on this and putting together a PR, appreciate the effort

After digging into it a little, I think the fix needs to go in a different direction. Here's the thing:

The user in the issue actually did set the environment variables (QISKIT_IBM_TOKEN and QISKIT_IBM_INSTANCE) exactly as the README says, so this isn't really a docs gap. The real problem is that the Rust code in crates/client/src/service.rs goes straight to reading the JSON config file and crashes if it doesn't exist, without ever checking the env vars, which should be expected behaviour, according to existing docs.

So I'm assuming the fix would actually need to be a code change

  1. Check env vars first (QISKIT_IBM_TOKEN, QISKIT_IBM_INSTANCE)
  2. Fall back to the JSON file if they're not set
  3. Return a friendly error message instead of panicking if neither is found, by changing the return type to a PyErr (a panic is unwanted, we should always aim for programs to exit gracefully with clear error messages for all public, user APIs)

Also worth noting, documenting the JSON file approach as the main path asks users to store their API token on disk, which isn't ideal from a security standpoint compared to env vars, and well, is also non-ergonomic.

I would suggest inspecting service.rs, handling the panic first (so it's also clear when you reproduce the error, if the code within this is the culprit), if yes, proceed with handling environment variables.

Let me know if you face any issues doing any of these steps, you can read up on https://doc.rust-lang.org/book/ch09-00-error-handling.html https://pyo3.rs/main/doc/pyo3/struct.pyerr https://doc.rust-lang.org/book/ch12-05-working-with-environment-variables.html

To understand syntax and strategy to do this. Do let me know if you face any bottlenecks

Thank you Aaryav (@aaryav-3) , I am working on it. This is solution is better.

@CedrickMwamba
Cedrick Mwamba (CedrickMwamba) marked this pull request as ready for review March 26, 2026 20:21

@aaryav-3 Aaryav (aaryav-3) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Cedrick Mwamba (@CedrickMwamba) thank you for making the changes, this was a great effort, just listing out a few comments and suggestions from my side.

Comment thread README.md
Comment thread README.md Outdated
Comment thread crates/client/src/service.rs Outdated
Comment thread crates/client/src/c_api.rs Outdated
Comment thread crates/client/src/c_api.rs Outdated
Comment thread crates/client/src/service.rs Outdated
Comment thread crates/client/src/service.rs Outdated
Comment thread CMakeLists.txt Outdated
Comment thread samples/test_error_handling_run.c
Comment thread samples/test_error_handling_run.c Outdated
@CedrickMwamba

Copy link
Copy Markdown
Author

Hi Cedrick Mwamba (@CedrickMwamba) thank you for making the changes, this was a great effort, just listing out a few comments and suggestions from my side.

Hi Aaryav (@aaryav-3) , thank you for your feedback. I will apply these changes.

@CedrickMwamba

Copy link
Copy Markdown
Author

Hi Aaryav (@aaryav-3) ,
it is ready again for review. I did most of the suggested changes.
Thank you.

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.

sampler_test/transpile_test aborted with No such file or directory error.

3 participants