Skip to content

Add tags instance filtering #2277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jul 1, 2025
Merged

Add tags instance filtering #2277

merged 14 commits into from
Jul 1, 2025

Conversation

kt474
Copy link
Member

@kt474 kt474 commented Jun 3, 2025 •
edited
Loading

Summary

We decided to just add filtering instances by tags, not resource group. Also updated the error handlings so an error will be raised at service initialization if no instances match the filter params given.

Details and comments

Fixes #2259

@kt474 kt474 marked this pull request as ready for review June 11, 2025 16:02
ElePT
ElePT reviewed Jun 12, 2025
View reviewed changes
Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks @kt474! I have a couple of questions after reviewing the PR:

  • the "resource_group" preference is set with the resource group ID, right? (I actually don't even know where to look for that)
  • what will happen if someone sets a tag or resource group that doesn't exist? Do we know what is the intended behavior?
  • the resource group unit test seems to be missing

@kt474
Copy link
Member Author

kt474 commented Jun 12, 2025 •
edited
Loading

  • the "resource_group" preference is set with the resource group ID, right? (I actually don't even know where to look for that)

I can only see the resource_group_id returned from the cloud global search API. I'm not sure if this is useful or we want to have the group name too? I think just having the id would not be helpful because as you mentioned, it's not even shown in the UI.

  • what will happen if someone sets a tag or resource group that doesn't exist? Do we know what is the intended behavior?

I think we should refactor _filter_instances_by_saved_preferences to just error if no matching backends are found across all of the preference related parameters. Currently this is only the case with plans_preference.

@francabrera what do you think?

Update: We're just going to add filtering by tags

@kt474 kt474 added the Changelog: New Feature Include in the Added section of the changelog label Jun 16, 2025
@kt474 kt474 changed the title Add tags and resource group instance preferences Add tags instance filtering Jun 19, 2025
@kt474 kt474 requested a review from ElePT June 19, 2025 19:45
ElePT
ElePT reviewed Jun 23, 2025
View reviewed changes
Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

The changes look good but I left a couple of questions regarding tests. Ideally, we should be able to mock the tag, plan_preference and region to test the backend retrieval directly, right?

@kt474
Copy link
Member Author

kt474 commented Jun 24, 2025

The changes look good but I left a couple of questions regarding tests. Ideally, we should be able to mock the tag, plan_preference and region to test the backend retrieval directly, right?

Yeah we should be able to mock everything

@kt474 kt474 requested a review from ElePT June 25, 2025 15:50
ElePT
ElePT previously approved these changes Jun 26, 2025
View reviewed changes
Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM! thanks a lot. Just to quickly confirm, in the case where a list of tags contains a "correct" and an "incorrect" tag, as long as a backend is found through the correct tag, there isn't any error being raised, right? Would it make sense to raise a warning mentioning that no backends were found for a specific tag?

@kt474
Copy link
Member Author

kt474 commented Jun 26, 2025

LGTM! thanks a lot. Just to quickly confirm, in the case where a list of tags contains a "correct" and an "incorrect" tag, as long as a backend is found through the correct tag, there isn't any error being raised, right? Would it make sense to raise a warning mentioning that no backends were found for a specific tag?

Currently, all tags have to match for backends to be returned. I believe this is the same behavior as the job_tags in the jobs() method. Do you think backends should be returned as long as there is one matching tag?

@ElePT
Copy link
Collaborator

ElePT commented Jun 27, 2025

I think that the filter should fail if there is any incorrect tag in the list, just as you described. I was asking because I think that my early tests of plans_preference would not fail when there was an incorrect plan together with a correct plan. I'm just thinking that we should do a quick check of all filters to make sure that the behavior is more or less consistent.

@kt474
Copy link
Member Author

kt474 commented Jun 27, 2025 •
edited
Loading

Yeah plans_preference works a little differently because it orders the instances based on the plans in the given list. Having an incorrect plan with a correct plan would not raise any errors - I'll update the release notes and tests so we can make the behavior clear.

ElePT
ElePT approved these changes Jul 1, 2025
View reviewed changes
Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM, I like the more indicative reno

@kt474 kt474 enabled auto-merge July 1, 2025 14:57
@kt474 kt474 added this pull request to the merge queue Jul 1, 2025
Merged via the queue into Qiskit:main with commit f2b8e20 Jul 1, 2025
17 checks passed
@kt474 kt474 deleted the resource-group-tags-pref branch July 1, 2025 17:04
kt474 added a commit that referenced this pull request Jul 6, 2025
@SamFerracin @kt474
@q-inho @jyu00 @ElePT
Merge main into box_preview (#2306)
d65f00c 
* Add tags instance filtering (#2277)

* wip add tags & resource group instance prefs

* add resource_group preference

* add reno

* add test

* remove resource group & raise error correctly

* reno typo

* update integration test

* add unit test

* update tests and release note

* Fix small typos (#2299)

* fix typo in base_runtime_job.py

estimation infromation for this job. -> estimation information for this job.

* fix typo in runtime_job_v2.py

primitive exeuction. -> primitive execution.

* fix typo in noise_learner.py

layers that occurr more frequently. -> layers that occur more frequently.

* fix typo (#2302)

* Handle new IQP inconsistencies/breaking changes  (#2296)

* new IQP breaking changes

* remove delete_job test

* add release note

* remove instance RuntimeOption

* deprecate properly

* typo

* fix reno & runtimeoptions, add test

* address comments

* Add queue_usage deprecation

---------

Co-authored-by: ElePT <epenatap@gmail.com>

* black

---------

Co-authored-by: Kevin Tian <kevin.tian@ibm.com>
Co-authored-by: Inho Choi <79438062+q-inho@users.noreply.github.com>
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
Co-authored-by: ElePT <epenatap@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElePT ElePT ElePT approved these changes

Assignees
No one assigned
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Selection of instances to filter instances by resource group or tags
2 participants
@kt474 @ElePT