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

clp-package: Fix bugs introduced in #460: #481

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Conversation

haiqi96
Copy link
Contributor

@haiqi96 haiqi96 commented Jul 15, 2024

Description

This PR fixes two bugs in query worker scripts, introduced by #460

  1. Fixes a bug where the search results are written to task_id collection. They should be written to job_id collection
  2. Fixes a bug where an int is converted to string before joined as part of the command

Validation performed

Tested with log viewer.

@kirkrodrigues kirkrodrigues requested review from kirkrodrigues and removed request for kirkrodrigues July 15, 2024 15:02
Copy link
Collaborator

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

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

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

@haiqi96
Copy link
Contributor Author

haiqi96 commented Jul 15, 2024

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

@junhaoliao
Copy link
Collaborator

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

Right, sorry for the oversight. How about: clp-package: Fix issues with command generation and search results write destination in query worker scripts.

@haiqi96
Copy link
Contributor Author

haiqi96 commented Jul 15, 2024

Quote reply
Reference in new issue
Edit
Hide
Delete

Lgtm. How about changing the PR title to "clp-package: Fix type conversions in query worker scripts."?

hmm, it's not only type conversion. the change also updated the result collection to use job_id instead of task_id.

Right, sorry for the oversight. How about: clp-package: Fix issues with command generation and search results write destination in query worker scripts.

@kirkrodrigues any suggestions?

@kirkrodrigues
Copy link
Member

How about:

clp-package: Fix bugs introduced in #460:

  • Write search results to collection named job_id rather than task_id.
  • Convert int to str in IR extraction command generation.

Ordinarily, I'd make these two separate PRs so the commit message is easier to write, but above is how I'd write it so that it makes sense why we're fixing both bugs in one PR.

@haiqi96 haiqi96 changed the title clp-package: Fix several bugs in query worker scripts clp-package: Fix bugs introduced in #460: Jul 15, 2024
@haiqi96 haiqi96 merged commit 862fccf into y-scope:main Jul 15, 2024
4 checks passed
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
- Write search results to collection named job_id rather than task_id.
- Convert int to str in IR extraction command generation.
@haiqi96 haiqi96 deleted the bug_fixes branch December 6, 2024 20:32
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.

3 participants