-
Notifications
You must be signed in to change notification settings - Fork 19
Refreshing tickets, cutoff date, logging updates, bug fixes #365
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
base: main
Are you sure you want to change the base?
Conversation
haozturk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bug identified, changes needed
|
I think we need to think a bit more about the document viewing/uploader. One reason to keep the account creation central was to control who was given the ability to add/delete documents (like you mentioned is not handled now). In general, I can think of a few permutations in which you might want the viewer to be accessible to everyone or only some, and then the same with actual uploading permissions (deploying to admins vs users, for example). Is it a huge pain to move the account creation code to a different PR? Since ticket updating is more pressing, whereas the uploader is not, and can move this discussion/additional changes to the new PR? Let me know @juanpablosalas With respect to the updating of the tickets, thanks a lot for getting this done quick, it is definitely essential for the deployment! |
1a81925 to
0ee17ec
Compare
|
Thanks Juan, my biggest concern with the current way of refreshing tickets is that |
|
Getting the following error now @juanpablosalas Use |
|
There's a bug and a various points to improve @juanpablosalas . Let me know when they are addressed and I'll review again |
|
Thanks for the thorough feedback @haozturk , I pushed the changes |
| try: | ||
| self.data_manager.update_tickets() | ||
| except Exception as e: | ||
| #NOTE: Error is logged but a failure to update tickets does not necessarily mean A2rchi cannot process and respond to the message | ||
| logger.error(f"Failed to update tickets - {str(e)}") | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to have an indicator should the ticket updating go wrong? for example if someone asks about a new ticket that came in but archi fails to grab it, to let the user know this so they know it's not an issue of retrieval/query or something? e.g., "failed to grab tickets since x time" or "last ticket update at x time"... what do you think? @juanpablosalas @haozturk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea to show last data update, but without communicating what data we're talking about, i don't think it would be useful. Tickets aren't the only data we have, so ideally we want to show the last doc update as well. I wouldn't include this feature in this PR, but rather make an issue and tackle it later. I will let Juan comment as well. If he agrees, we should make an issue for this.
pmlugato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juanpablosalas thanks a lot for the good work and the quick iteration with @haozturk . I've left a small comment which I'll leave up to you both as operators whether it would be useful to include or not. Other, than that, the rest looks great to me, Hasan feel free to merge when you are ready!
| return | ||
|
|
||
| self.client = client | ||
| self.timezone = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be self.jira_timezone = None, o/w it fails :(
(2025-12-15 09:29:40,199) [src.interfaces.chat_app.app] ERROR: Failed to update tickets - 'JiraClient' object has no attribute 'jira_timezone'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and ran it again with no issues
|
Thanks @juanpablosalas can you please resolve the conflicts that arose after #354 is merged |
config changes
4bb031a to
de5cf66
Compare
|
Tested locally, for some reason refreshing capability doesn't work anymore. Not ready to be merged. Discussing with Juan what's wrong. |
11eb37d to
a1e03fa
Compare
added documentation
fixed bug when no frequency is passed in the config always update fixed when jira frequency is 0 undo document index changes
fixed requested changes improved debug message and timezone errors fixed jira created and updated dates
fixed jira_timezone declaration fix bug fixed issues when rebasing removed utils again re added changes
a1e03fa to
f6a7e85
Compare
|
Hi @juanpablosalas I am afraid that my bad coding practices means there are going to be a lot of conflicts with my PR #377. Do you mind if we close this, and I will merge the relevant changes into #377? |
|
Not at all @lucalavezzo, in any case I was aware the changes will need to be reconciled given the introduction of the cronjob infrastructure you are developing! |
|
I was about to test this and merge if it was okay. Should I not touch it then? |
|
Hi @lucalavezzo ofc, please let me know when yours is ready for a review. I'm curious to check it. |
This PR introduces a series of changes and bug fixes mainly to the handling of tickets.
Tickets
ChatWrapper. If the frequency is set to 0 it will pull the tickets again on every call. (See Refresh JIRA tickets since last pull #359)Logging
persistence.py,vectorstore/manager.py. A list of stored resources was being logged as info but I find this to make a lot of noise (specially considering there are hundreds of tickets in each project, not to mention the crawled pages). I reduced the logging level to DEBUG so it's not completely gone.httpxmoduleBug fixes/Code cleaning