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

Add support for scheduled refresh. #33

Closed

Conversation

kghamilton89
Copy link
Contributor

Resolves reported issue.

Changes already live in Dutch MPEP dashboard Space and Russian MPEP dashboard Space.

Humble suggestion to add support for automated refresh out of the box.

@alvarobartt
Copy link
Member

cc @ignacioct

@ignacioct
Copy link
Contributor

Hi @kghamilton89, thanks for the PR. This code looks very similar to the code we initially had to allow background refresh, but it was giving us a lot of problems. The dashboard was getting frozen fairly regularly. Are you not experiencing this?

If it's working on your side, we could maybe add it as an option or comments in the code, but I and @dvsrepo are skeptics about the BackgroundScheduler usage.

@kghamilton89
Copy link
Contributor Author

kghamilton89 commented Mar 24, 2024

Hi @kghamilton89, thanks for the PR. This code looks very similar to the code we initially had to allow background refresh, but it was giving us a lot of problems. The dashboard was getting frozen fairly regularly. Are you not experiencing this?

If it's working on your side, we could maybe add it as an option or comments in the code, but I and @dvsrepo are skeptics about the BackgroundScheduler usage.

In fact yes, sometimes I see that dashboard hangs when I leave it open for a period of time. Use case: I open dashboard and leave it open for an arbitrary period of time. I return to dash and see "error" for all visualizations. (confirmed that this occurs for our Dutch-speaking colleagues in same use case)

Refresh always resolves this issue and looking at console shows 500 error for Argilla servers.

Do you have any hypothesis for why this happens? As I say in here expecting Lang. Leads to restart spaces manually anytime they want updates seems to me to be the greater of two evils compared to **sometimes breaking dash when left open for extended periods of time.

Maybe there is an issue with Argilla / HF Spaces integration? For example setting share=True in launch() returns that such a feature is not supported in HF Spaces. Maybe this indicates that other features are not supported also?

Agree that we need to identify underlying cause and resolve. Happy to investigate and resolve before we merge this.

@ignacioct
Copy link
Contributor

here

Hi @kghamilton89, thanks for the PR. This code looks very similar to the code we initially had to allow background refresh, but it was giving us a lot of problems. The dashboard was getting frozen fairly regularly. Are you not experiencing this?
If it's working on your side, we could maybe add it as an option or comments in the code, but I and @dvsrepo are skeptics about the BackgroundScheduler usage.

In fact yes, sometimes I see that dashboard hangs when I leave it open for a period of time. Use case: I open dashboard and leave it open for an arbitrary period of time. I return to dash and see "error" for all visualizations. (confirmed that this occurs for our Dutch-speaking colleagues in same use case)

Refresh always resolves this issue and looking at console shows 500 error for Argilla servers.

Do you have any hypothesis for why this happens? As I say in here expecting Lang. Leads to restart spaces manually anytime they want updates seems to me to be the greater of two evils compared to **sometimes breaking dash when left open for extended periods of time.

Maybe there is an issue with Argilla / HF Spaces integration? For example setting share=True in launch() returns that such a feature is not supported in HF Spaces. Maybe this indicates that other features are not supported also?

Agree that we need to identify underlying cause and resolve. Happy to investigate and resolve before we merge this.

Yep, you're getting the same error as we were getting. And we still have no clue why it is happening. Given the circumstances, we decided to manually update the dashboards from time to time, and we also looked into options for automatically resetting the space, which is possible. If you wanna go down the rabbit hole, it would be nice, but I personally believe it's worth it to look into how to automatically restart the spaces; I don't think having live, updated dashboards it's a priority, even though it'll be really nice.

@kghamilton89
Copy link
Contributor Author

Fully understood @ignacioct, thanks for the feedback. I'll have a look and see what I can gather. In the meantime, closing PR.

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