drop the gui bus upload of resources #53
Conversation
WalkthroughThe changes in this pull request involve the reorganization and enhancement of documentation in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- README.md (0 hunks)
- ovos_gui/constants.py (1 hunks)
- ovos_gui/gui_file_server.py (0 hunks)
- ovos_gui/namespace.py (7 hunks)
- ovos_gui/page.py (2 hunks)
- test/unittests/test_namespace.py (1 hunks)
Files not reviewed due to no reviewable changes (2)
- README.md
- ovos_gui/gui_file_server.py
Additional context used
Ruff
ovos_gui/namespace.py
955-955: f-string without any placeholders
Remove extraneous
fprefix(F541)
Additional comments not posted (14)
ovos_gui/constants.py (2)
1-1: LGTM!The import statement is correctly formatted and the imported function is used to define the
GUI_CACHE_PATHconstant.
3-3: LGTM!The
GUI_CACHE_PATHconstant is correctly defined using theget_xdg_cache_save_pathfunction. The constant is named using theUPPER_CASEconvention, which is a best practice for constants in Python. The constant is used to define the cache directory path for theovos_guiapplication.ovos_gui/page.py (4)
5-5: LGTM!The import statement is correctly added.
48-50: LGTM!The
_gui_cacheproperty is correctly defined and uses the appropriate variables to construct the cache path.
52-54: LGTM!The
res_namespaceproperty correctly determines the namespace based on thepage_idattribute.
56-76: LGTM!The refactored
get_urimethod improves clarity and maintainability by:
- Removing the
server_urlparameter and associated logic.- Utilizing the
res_namespaceproperty to determine the appropriate path construction.- Using the
_gui_cacheproperty for consistent cache path structure.- Including a logging statement for debugging purposes.
- Maintaining the original error handling behavior.
test/unittests/test_namespace.py (1)
Line range hint
470-479: LGTM! The test aligns with the PR objective.The changes in the test function
test_upload_system_resourcesreflect the shift from uploading to caching system resources, which aligns with the PR objective of eliminating the GUI bus upload mechanism.The test logic and assertions remain valid for verifying the caching behavior of the
_cache_system_resourcesmethod.ovos_gui/namespace.py (7)
Line range hint
48-58: LGTM!The import statements are well-organized and the newly added imports seem relevant to the module.
449-452: LGTM!The initialization of the GUI file server based on the configuration looks good. Using the
GUI_CACHE_PATHas the root directory for serving files is a sensible approach.
704-707: LGTM!Adding the check for active namespaces before processing the 'load_pages' request is a good defensive programming practice. The error logging will help in identifying and fixing issues related to unexpected states.
878-878: LGTM!Including the
frameworkkey in themycroft.gui.portmessage data is a good addition. It provides useful information about the GUI framework being used, which can be leveraged for framework-specific handling or optimizations.
927-930: LGTM!Adding the check for active namespaces before processing the 'back' signal is a good defensive programming practice. The error logging will help in identifying and fixing issues related to unexpected states.
949-958: LGTM!The renaming of the
_upload_system_resourcesmethod to_cache_system_resourcesaccurately reflects the updated functionality of caching the system GUI resources. Removing existing resources before updating is a good practice to ensure a clean and consistent state. The resource copying logic looks straightforward, and the debug logging adds visibility into the caching process.Tools
Ruff
955-955: f-string without any placeholders
Remove extraneous
fprefix(F541)
953-953: LGTM!The f-string is used correctly to construct the output path for caching system resources. The interpolation of the
GUI_CACHE_PATHvariable looks good, and the resulting path seems appropriate.
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
JarbasAl
force-pushed
the
drop_bus_upload
branch
from
8d49159
to
9f2427e
Compare
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
companion to OpenVoiceOS/ovos-gui#53 and OpenVoiceOS/ovos-bus-client#120 this should be merged first as it does not require the others to work and does not break anything in the wild
|
This is configurable, I would prefer a deprecation warning before removing the functionality completely (maybe in 1.0.0?) since it is a breaking change. |
|
its marked as breaking and we follow semver, so should not affect anything, besides this is not working properly so whats is there to keep compat with? the bus upload was introduced to make it work in docker, and it isnt working properly there, it is a failed attempt that never worked right, this change is meant to solve that there (still being tested) |
|
and yes, this is going to be 1.0.0 because of semver automations |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
README.md (2)
40-40: Approved changes, but consider adding more context.The removal of file server and GUI file collection options aligns with the PR objectives to address CPU spikes and OOM errors. The added note about QT5 clients is helpful.
Consider adding a brief explanation about why these options were removed and how it affects the system's performance and stability. This would provide valuable context for users and maintainers.
Line range hint
1-58: Consider adding important implementation details.The README currently doesn't mention two crucial points from the PR description:
- The bus client needs to be updated in all containers for this change to work properly.
- The
~/.cache/ovos_guidirectory needs to be a shared volume.Consider adding a section to the README that outlines these important implementation details. This will help users and maintainers understand the full scope of changes required when implementing this update.
ovos_gui/namespace.py (1)
297-299: Optimize List Comprehension Usage in Page Names CheckIn the
load_pagesmethod, the expression[p.name for p in self.pages]is recalculated in each iteration of the loop, which is inefficient.To improve performance, calculate the list of existing page names once before the loop:
existing_page_names = [p.name for p in self.pages] for page in pages: if page.name not in existing_page_names: new_pages.append(page)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- README.md (1 hunks)
- ovos_gui/gui_file_server.py (0 hunks)
- ovos_gui/namespace.py (10 hunks)
💤 Files with no reviewable changes (1)
- ovos_gui/gui_file_server.py
🧰 Additional context used
🪛 Ruff
ovos_gui/namespace.py
910-910: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (1)
README.md (1)
Line range hint
1-58: Overall changes approved with suggestions for improvement.The modifications to the README accurately reflect the removal of the GUI bus upload mechanism, which aligns with the PR objectives to address CPU spikes and OOM errors. The remaining configuration instructions are clear and concise.
To further improve the documentation:
- Consider adding a brief explanation of why certain options were removed and how it benefits system performance.
- Include information about updating bus clients in all containers and the need for a shared volume for
~/.cache/ovos_gui.These additions would provide a more comprehensive guide for users implementing these changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
README.md (4)
42-53: LGTM: Valuable plugin information addedThe new section on plugins provides useful information about platform-specific GUI functionality. This addition improves the documentation and aligns with the PR's objective of clarifying system components.
Consider formatting the URLs as proper markdown links for better readability:
- [OVOS GUI Plugin Shell Companion](https://github.com/OpenVoiceOS/ovos-gui-plugin-shell-companion) - [OVOS GUI Plugin Mobile](https://github.com/OpenVoiceOS/ovos-gui-plugin-mobile) - [OVOS GUI Plugin Plasmoid](https://github.com/OpenVoiceOS/ovos-gui-plugin-plasmoid) - [OVOS GUI Plugin Bigscreen](https://github.com/OpenVoiceOS/ovos-gui-plugin-bigscreen)🧰 Tools
🪛 Markdownlint
49-49: null
Bare URL used(MD034, no-bare-urls)
50-50: null
Bare URL used(MD034, no-bare-urls)
51-51: null
Bare URL used(MD034, no-bare-urls)
52-52: null
Bare URL used(MD034, no-bare-urls)
55-58: LGTM: Important limitations section addedThe new "Limitations" section provides crucial information about GUI resource files and their accessibility. This addition directly addresses the PR objectives by clarifying system limitations and the new approach to resource management.
There's a small typo in the word "expectd" on line 57. Please correct it to "expected":
-gui resources files are populated under `~/.cache/mycrot/ovos-gui` by skills and other OVOS components and are expectd to be accessible by GUI client applications +gui resources files are populated under `~/.cache/mycrot/ovos-gui` by skills and other OVOS components and are expected to be accessible by GUI client applications
59-61: LGTM: Valuable information on GUI client expectations and future plansThis section provides important details about GUI client expectations and includes a TODO note about future improvements. This information aligns well with the PR objectives by explaining the new approach to resource management.
Consider a minor language improvement in line 59:
-This means GUI clients are expected to be running under the same machine or implement their own access to the resource files (resolving page names to uris is the client app responsibility) +This means GUI clients are expected to be running on the same machine or implement their own access to the resource files (resolving page names to uris is the client app responsibility)The preposition "on" is more commonly used when referring to software running on a machine.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~59-~59: The preposition “on” seems more likely in this position.
Context: ... GUI clients are expected to be running under the same machine or implement their own...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
63-64: LGTM: Important information for container usage addedThis section provides crucial information for users working with containers, which aligns well with the PR objectives and helps clarify the new resource management approach.
Consider these minor grammatical improvements:
-In case of containers a shared volume should be mounted between ovos-gui, skills and gui client apps +In the case of containers, a shared volume should be mounted between ovos-gui, skills and gui client appsThese changes improve readability and adhere to common English grammar conventions.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...s-gui` to be handled by client apps In case of containers a shared volume should be...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~63-~63: Consider adding a comma here.
Context: ...o be handled by client apps In case of containers a shared volume should be mounted betwe...(IN_CASE_OF_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~59-~59: The preposition “on” seems more likely in this position.
Context: ... GUI clients are expected to be running under the same machine or implement their own...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...s-gui` to be handled by client apps In case of containers a shared volume should be...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~63-~63: Consider adding a comma here.
Context: ...o be handled by client apps In case of containers a shared volume should be mounted betwe...(IN_CASE_OF_COMMA)
🪛 Markdownlint
README.md
49-49: null
Bare URL used(MD034, no-bare-urls)
50-50: null
Bare URL used(MD034, no-bare-urls)
51-51: null
Bare URL used(MD034, no-bare-urls)
52-52: null
Bare URL used(MD034, no-bare-urls)
🔇 Additional comments (2)
README.md (2)
29-29: LGTM: Informative comment addedThe added comment provides valuable information about the current state of GUI clients. This aligns well with the PR's objective of improving clarity and addressing system limitations.
Line range hint
1-64: Overall changes align well with PR objectivesThe modifications to the README.md file effectively support the PR's main objectives:
- The removal of file server configuration options aligns with the goal of eliminating the problematic GUI bus upload mechanism.
- The new sections on limitations and container usage provide clear guidance on the new approach to resource management.
- The added information about plugins and GUI client expectations improves overall documentation and clarity.
These changes contribute to addressing the issues of CPU spikes and OOM errors by providing a clearer understanding of the system's new architecture and limitations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~59-~59: The preposition “on” seems more likely in this position.
Context: ... GUI clients are expected to be running under the same machine or implement their own...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
[uncategorized] ~63-~63: You might be missing the article “the” here.
Context: ...s-gui` to be handled by client apps In case of containers a shared volume should be...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~63-~63: Consider adding a comma here.
Context: ...o be handled by client apps In case of containers a shared volume should be mounted betwe...(IN_CASE_OF_COMMA)
🪛 Markdownlint
49-49: null
Bare URL used(MD034, no-bare-urls)
50-50: null
Bare URL used(MD034, no-bare-urls)
51-51: null
Bare URL used(MD034, no-bare-urls)
52-52: null
Bare URL used(MD034, no-bare-urls)
|
|
||
| This means GUI clients are expected to be running under the same machine or implement their own access to the resource files (resolving page names to uris is the client app responsibility) | ||
|
|
||
| > TODO: new repository with the removed GUI file server, serve files from `~/.cache/mycrot/ovos-gui` to be handled by client apps |
There was a problem hiding this comment.
@NeonDaniel not sure if you had a specific use case in mind, but please see this note
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
test/unittests/test_bus.py (1)
83-94: LGTM! Consider adding the missing import forList.The implementation of
_get_client_pageslooks good. It correctly iterates over the pages in the namespace and resolves each page to a full URI.To resolve the static analysis warning, consider adding the following import at the beginning of the file:
from typing import List🧰 Tools
🪛 Ruff
83-83: Undefined name
List(F821)
ovos_gui/bus.py (4)
142-148: Approve changes with a minor suggestion for comment clarity.The modifications to the
synchronizemethod align well with the PR objectives of dropping the GUI bus upload of resources. This change should help address the CPU spikes and OOM errors mentioned in the PR description.Consider clarifying the comment slightly for better readability:
-# if uri (path) can not be resolved, it might exist client side -# if path doesn't exist in client side, client is responsible for resolving page by namespace/name +# If the URI (path) cannot be resolved server-side, it might exist client-side. +# If the path doesn't exist client-side, the client is responsible for resolving the page by namespace/name.
250-256: Approve changes with a suggestion for code improvement.The modifications to the
send_gui_pagesmethod are consistent with the changes made in thesynchronizemethod and support the PR objectives of streamlining the GUI resource handling.Consider extracting the repeated comment and page data structure into a shared constant or utility method to reduce code duplication:
def get_page_data(pages, framework): """ If the URI (path) cannot be resolved server-side, it might exist client-side. If the path doesn't exist client-side, the client is responsible for resolving the page by namespace/name. """ return [{"url": page.get_uri(framework), "page": page.name} for page in pages] # In synchronize method data = get_page_data(namespace.pages, self.framework) # In send_gui_pages method data = get_page_data(pages, framework)This refactoring would improve maintainability and ensure consistency across both methods.
Line range hint
1-278: Summary: Changes effectively address PR objectivesThe modifications made to
ovos_gui/bus.pyalign well with the PR objectives of dropping the GUI bus upload of resources. These changes should help address the CPU spikes and out-of-memory (OOM) errors mentioned in the PR description by simplifying the GUI resource handling process.Key improvements:
- Streamlined page data sending in both
synchronizeandsend_gui_pagesmethods.- Removal of the
get_client_pagesmethod, reducing unnecessary data transfer.- Shifting responsibility for page resolution to the client when necessary.
These changes appear to be a step in the right direction for improving system stability and performance. However, as mentioned in the PR comments, thorough testing is crucial to ensure that these modifications work as expected across all scenarios.
To fully realize the benefits of these changes:
- Ensure that the bus client is updated in all containers, as mentioned in the PR description.
- Verify that
~/.cache/ovos_guiis set up as a shared volume where necessary.- Consider adding logging or monitoring to track the performance improvements resulting from these changes, which could help validate the effectiveness of this new approach.
References to
get_client_pagesfound in test files.
test/unittests/test_bus.pycontains references toget_client_pagesthat need to be updated or removed to match the recent changes.Please address these test references to prevent potential regressions.
🔗 Analysis chain
Line range hint
1-278: Approve removal ofget_client_pagesmethod with a verification suggestion.The removal of the
get_client_pagesmethod aligns with the PR objectives of dropping the GUI bus upload of resources. This change should contribute to resolving the CPU spikes and OOM errors mentioned in the PR description.To ensure this removal doesn't introduce any regressions, please run the following verification script:
If the script returns any results, those occurrences should be reviewed and updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removed method is not referenced elsewhere in the codebase. # Test: Search for any remaining references to 'get_client_pages'. Expect: No results. rg --type python 'get_client_pages'Length of output: 66
Script:
#!/bin/bash # Description: Verify that the removed method is not referenced elsewhere in the codebase. # Test: Search for any remaining references to 'get_client_pages'. Expect: No results. rg 'get_client_pages'Length of output: 333
ovos_gui/page.py (1)
43-43: Update docstring to reflect supported frameworksThe docstring states that only
'qt5'is supported, but theget_file_extensionmethod handles both'qt5'and'qt6'. Consider updating the docstring to include'qt6'as a supported framework.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- ovos_gui/bus.py (2 hunks)
- ovos_gui/page.py (2 hunks)
- test/unittests/test_bus.py (2 hunks)
🧰 Additional context used
🪛 Ruff
test/unittests/test_bus.py
83-83: Undefined name
List(F821)
🔇 Additional comments (2)
test/unittests/test_bus.py (2)
110-120: LGTM! The test method has been updated correctly.The changes to
test_get_client_pagesappropriately utilize the new_get_client_pagesmethod while maintaining the existing test scenarios. The removal of commented-out test cases helps in keeping the test file clean and focused.
Line range hint
83-120: Summary: Improved code organization and maintainabilityThe changes in this file contribute to the PR's objective of streamlining GUI resource management. The introduction of the
_get_client_pagesmethod encapsulates the logic for retrieving client page URLs, improving code organization and readability. The test cases have been updated accordingly, maintaining proper coverage while simplifying the overall structure.These modifications enhance the maintainability of the code without introducing breaking changes or performance issues. They align well with the goal of refining the GUI resource handling process.
🧰 Tools
🪛 Ruff
83-83: Undefined name
List(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
protocol.md (2)
64-67: Approve the addition with a minor grammatical correction.This new paragraph provides crucial information about handling GUI resources that cannot be resolved to a URL. It aligns well with the PR objectives of improving GUI resource management.
Consider applying this minor grammatical correction:
-If a gui resource can not be resolved to a url (*url* may be `None`!), it might still exist client side, it is the clients responsibility to handle the namespace/page in that case +If a GUI resource cannot be resolved to a URL (*url* may be `None`!), it might still exist client-side. It is the client's responsibility to handle the namespace/page in that case.🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
Line range hint
1-274: Summary of protocol documentation changes.The changes to the protocol documentation are well-aligned with the PR objectives of improving GUI resource management and addressing issues related to CPU spikes and OOM errors. The additions and modifications provide:
- Clarification on handling GUI resources that cannot be resolved to a URL.
- Updated structure for inserting new pages, adding more specificity to page identification.
These changes should contribute to more robust and efficient GUI handling in the OVOS system.
Consider adding a section or note about the performance implications of these changes, specifically addressing how they help mitigate the CPU spikes and OOM errors mentioned in the PR objectives. This would provide valuable context for developers implementing or maintaining the protocol.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
test/unittests/test_namespace.py (1)
439-450: LGTM: Improved system resource caching testThe changes to the
test_upload_system_resourcesmethod significantly improve the test coverage for the system resource caching mechanism. The use ofGUI_CACHE_PATH, clearing the cache before testing, and specific assertions for cached resources enhance the test's effectiveness.Consider adding a small improvement:
To ensure complete cleanup after the test, consider adding
rmtree(p)within afinallyblock. This will ensure the test directory is always cleaned up, even if an assertion fails. Here's a suggested modification:def test_upload_system_resources(self): p = f"{GUI_CACHE_PATH}/system" - rmtree(p) - self.namespace_manager._cache_system_resources() - self.assertTrue(isdir(join(p, "qt5"))) - self.assertTrue(isfile(join(p, "qt5", - "SYSTEM_TextFrame.qml"))) - # Test repeated copy doesn't raise any exception - self.namespace_manager._cache_system_resources() - self.assertTrue(isdir(join(p, "qt5"))) - self.assertTrue(isfile(join(p, "qt5", - "SYSTEM_TextFrame.qml"))) - rmtree(p) + try: + rmtree(p) + self.namespace_manager._cache_system_resources() + self.assertTrue(isdir(join(p, "qt5"))) + self.assertTrue(isfile(join(p, "qt5", + "SYSTEM_TextFrame.qml"))) + # Test repeated copy doesn't raise any exception + self.namespace_manager._cache_system_resources() + self.assertTrue(isdir(join(p, "qt5"))) + self.assertTrue(isfile(join(p, "qt5", + "SYSTEM_TextFrame.qml"))) + finally: + rmtree(p)This ensures that the test directory is always cleaned up, improving test isolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- protocol.md (1 hunks)
- test/unittests/test_bus.py (4 hunks)
- test/unittests/test_gui_file_server.py (0 hunks)
- test/unittests/test_namespace.py (7 hunks)
- test/unittests/test_page.py (0 hunks)
💤 Files with no reviewable changes (2)
- test/unittests/test_gui_file_server.py
- test/unittests/test_page.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unittests/test_bus.py
🧰 Additional context used
🪛 LanguageTool
protocol.md
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🔇 Additional comments (8)
protocol.md (1)
74-74: Approve the change and verify implementation updates.The addition of the "page" key in the "values" array provides more specificity in page identification. This change is consistent with the PR objectives and the AI-generated summary.
Please ensure that both the server and client implementations have been updated to handle this new structure. Run the following script to verify the changes:
test/unittests/test_namespace.py (7)
25-25: LGTM: New import added for GUI_CACHE_PATHThe addition of this import is consistent with the changes made in the
test_upload_system_resourcesmethod. It's a good practice to import constants from a centralized location.
133-135: LGTM: Consistent GuiPage instantiation updatesThe changes in this method are consistent with the earlier modifications to
GuiPageinstantiations. The removal of theurlparameter maintains the simplified page creation process.
150-152: LGTM: Consistent GuiPage instantiation updatesThe changes in this method maintain consistency with the earlier modifications to
GuiPageinstantiations. The removal of theurlparameter continues to simplify the page creation process throughout the test suite.
175-177: LGTM: Consistent GuiPage instantiation updatesThe changes in the
test_remove_pagesmethod align with the earlier modifications toGuiPageinstantiations. The removal of theurlparameter maintains consistency in the simplified page creation process across various test scenarios.
262-262: LGTM: Consistent GuiPage instantiation updateThe change in the
test_handle_delete_page_active_namespacemethod is consistent with the earlier modifications toGuiPageinstantiations. The removal of theurlparameter maintains the simplified page creation process across different test contexts.
Line range hint
1-450: Summary of changesThe modifications in this file primarily focus on two aspects:
- Simplifying
GuiPageinstantiations by removing theurlparameter across all test methods.- Improving the system resource caching test by using
GUI_CACHE_PATHand implementing more specific assertions.These changes align with the PR objectives of addressing performance issues and improving system stability. The consistent updates to
GuiPageinstantiations suggest a broader change in theGuiPageclass implementation, which should be verified. The enhanced system resource caching test provides better coverage for this critical functionality.Overall, the changes appear to be well-implemented and consistent. They contribute to the goal of streamlining the GUI resource management process.
68-72: LGTM: Simplified GuiPage instantiationsThe removal of the
urlparameter fromGuiPageconstructor calls simplifies the page creation process. This change is consistent with the modifications observed throughout the file.Please ensure that the
GuiPageclass implementation has been updated to reflect this change. Run the following script to verify theGuiPageclass definition:
never worked right, causes more issues than it helps
JarbasAl
force-pushed
the
drop_bus_upload
branch
from
1a02acb
to
3105ddd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
README.md (3)
42-53: LGTM: Improved document structure with plugin information.The reorganization of the "Plugins" section enhances the overall document structure. However, consider improving the markdown formatting for the plugin links.
Consider updating the plugin links to use proper markdown link syntax:
- [OVOS GUI Plugin Shell Companion](https://github.com/OpenVoiceOS/ovos-gui-plugin-shell-companion) - [OVOS GUI Plugin Mobile](https://github.com/OpenVoiceOS/ovos-gui-plugin-mobile) - [OVOS GUI Plugin Plasmoid](https://github.com/OpenVoiceOS/ovos-gui-plugin-plasmoid) - [OVOS GUI Plugin Bigscreen](https://github.com/OpenVoiceOS/ovos-gui-plugin-bigscreen)🧰 Tools
🪛 Markdownlint
49-49: null
Bare URL used(MD034, no-bare-urls)
50-50: null
Bare URL used(MD034, no-bare-urls)
51-51: null
Bare URL used(MD034, no-bare-urls)
52-52: null
Bare URL used(MD034, no-bare-urls)
61-62: LGTM: Informative TODO note about future plans.This TODO note provides valuable information about future plans regarding the removed GUI file server. It aligns well with the PR objectives and helps developers understand the ongoing changes in the project.
Consider formatting the TODO as a task list item for better visibility:
- [ ] TODO: new repository with the removed GUI file server, serve files from `~/.cache/mycrot/ovos-gui` to be handled by client apps
63-64: LGTM: Important information about container usage.This addition provides crucial information for users working with containers, which is relevant to the changes in resource management.
Consider adding a comma after "In case of containers" for improved readability:
In case of containers, a shared volume should be mounted between ovos-gui, skills and gui client apps🧰 Tools
🪛 LanguageTool
[typographical] ~63-~63: Consider adding a comma here.
Context: ...o be handled by client apps In case of containers a shared volume should be mounted betwe...(IN_CASE_OF_COMMA)
test/unittests/test_bus.py (3)
83-94: LGTM: New helper method for tests, with a minor suggestion.The new
_get_client_pagesmethod is a good addition for test cases. It's well-documented and encapsulates the logic for retrieving page URIs.However, consider using
self.handler.frameworkinstead of hardcoding "qt5" on line 92. This would make the method more flexible for testing different frameworks.Suggested change:
- uri = page.get_uri("qt5") + uri = page.get_uri(self.handler.framework)
130-133: LGTM: Updated GuiPage mock objects with additional parameters.The changes to the GuiPage mock objects reflect updates to the GuiPage class constructor. This aligns with the PR's goal of improving GUI resource management.
Consider updating the documentation for the
test_send_gui_pagesmethod to explain the significance of these new parameters if they affect the test's behavior or assertions.
138-139: LGTM: Updated assertions for framework usage and data structure.The changes to the assertions in
test_send_gui_pagescorrectly verify the use of the appropriate framework when getting page URIs. The updated data structure, including both "url" and "page" keys, reflects the changes in GUI resource management mentioned in the PR objectives.Consider updating the
test_send_gui_pagesmethod documentation to explain the new data structure being sent to the GUI, particularly the inclusion of both URL and page identifier.Also applies to: 144-144, 149-150, 155-155
protocol.md (2)
64-67: Approve addition with minor grammatical fix.This addition provides valuable information for GUI client implementations, clarifying the client's responsibility when a GUI resource cannot be resolved to a URL. The example is helpful for developers.
Consider applying this minor grammatical fix:
-If a gui resource can not be resolved to a url (*url* may be `None`!), it might still exist client side, it is the clients responsibility to handle the namespace/page in that case +If a gui resource can not be resolved to a url (*url* may be `None`!), it might still exist client side, it is the client's responsibility to handle the namespace/page in that case🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
Line range hint
1-274: Overall, these changes improve the OVOS GUI service protocol documentation.The modifications in this file enhance the clarity and functionality of the OVOS GUI service protocol. They address the issues mentioned in the PR objectives, particularly regarding GUI resource handling and streamlining the process. The changes are well-aligned with the broader modifications across the codebase and contribute to a more robust and efficient GUI management system.
Consider adding a version number or last updated date to the protocol documentation to help track changes over time.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
ovos_gui/bus.py (3)
142-148: LGTM! Consider adding error handling for URI resolution.The changes simplify the page insertion logic and provide clear comments on the URI resolution process. This aligns well with the PR objectives of dropping the GUI bus upload of resources.
Consider adding error handling for cases where
page.get_uri(self.framework)might fail:try: uri = page.get_uri(self.framework) except Exception as e: LOG.warning(f"Failed to resolve URI for page {page.name}: {e}") uri = None # or some default value data = [{"url": uri, "page": page.name} for page in namespace.pages if uri is not None]This would make the code more robust against potential URI resolution failures.
256-257: LGTM! Consider consistent naming for clarity.The changes simplify the page data creation and align well with the PR objectives. For consistency with the
synchronizemethod, consider usingself.frameworkinstead offramework.Apply this minor change for consistency:
- "data": [{"url": page.get_uri(framework), "page": page.name} + "data": [{"url": page.get_uri(self.framework), "page": page.name} for page in pages]This ensures consistent use of
self.frameworkthroughout the class.
Incomplete Deprecation of GUI Bus Upload Mechanism
A TODO comment remains in
ovos_gui/namespace.pyrelated to the deprecation of the GUI bus upload mechanism. This indicates that the deprecation process is not fully complete and may require further action.
- Action Required: Remove or address the TODO comment in
ovos_gui/namespace.pyto ensure complete deprecation of the GUI bus upload mechanism.🔗 Analysis chain
Line range hint
1-277: Overall, the changes look good and align with the PR objectives.The modifications successfully simplify the code for handling GUI pages and align with the goal of dropping the GUI bus upload of resources. The added comments improve code clarity, and the changes are consistent across different methods.
A few minor suggestions have been made for error handling and naming consistency, but these are not critical issues. The changes appear to address the CPU spikes and OOM errors mentioned in the PR objectives by streamlining the resource handling process.
To ensure that these changes don't introduce any regressions, it would be beneficial to run the following verification:
This script will help ensure that all references to the old GUI bus upload mechanism have been removed and that the use of
self.frameworkis consistent throughout the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no references to the old GUI bus upload mechanism remain in the codebase. # Test: Search for any remaining references to GUI bus upload echo "Searching for references to GUI bus upload..." rg -i "gui.*bus.*upload" # Test: Check for any TODO comments related to GUI resource handling echo "Checking for TODO comments related to GUI resource handling..." rg -i "TODO.*gui.*resource" # Test: Verify consistent use of self.framework echo "Verifying consistent use of self.framework..." rg "framework" ovos_gui/bus.pyLength of output: 1278
Script:
#!/bin/bash # Description: Comprehensive search for remnants of the GUI bus upload mechanism. # Test 1: Search for any TODO comments related to GUI bus upload. echo "Searching for TODO comments related to GUI bus upload..." rg -i "TODO.*gui.*bus.*upload" # Test 2: Find all imports of namespace.py to ensure it's no longer in use. echo "Searching for imports of namespace.py..." rg 'import .*namespace' -- glob "*.py" # Test 3: Look for any remaining references to GUI bus upload in the codebase. echo "Searching for any references to GUI bus upload mechanism..." rg -i "gui.*bus.*upload"Length of output: 705
test/unittests/test_namespace.py (1)
433-444: LGTM with suggestions: Updated system resource caching test.The changes align with the PR's objective of improving resource management. The use of
GUI_CACHE_PATHand_cache_system_resources()suggests a more standardized approach to resource caching.However, consider the following suggestions to improve test isolation:
- Mock the
GUI_CACHE_PATHto use a temporary directory for testing.- Mock the
_cache_system_resources()method to avoid depending on the actual implementation.These changes would make the test more robust and less dependent on the actual file system.
Here's a suggested refactor to improve test isolation:
import tempfile from unittest.mock import patch class TestNamespaceManager(TestCase): # ... other methods ... @patch('ovos_gui.namespace.GUI_CACHE_PATH') def test_upload_system_resources(self, mock_cache_path): with tempfile.TemporaryDirectory() as temp_dir: mock_cache_path.return_value = temp_dir p = f"{temp_dir}/system" self.namespace_manager._cache_system_resources() self.assertTrue(isdir(join(p, "qt5"))) self.assertTrue(isfile(join(p, "qt5", "SYSTEM_TextFrame.qml"))) # Test repeated copy doesn't raise any exception self.namespace_manager._cache_system_resources() self.assertTrue(isdir(join(p, "qt5"))) self.assertTrue(isfile(join(p, "qt5", "SYSTEM_TextFrame.qml")))This approach uses a temporary directory and mocking to isolate the test from the actual file system and cache implementation.
ovos_gui/page.py (2)
43-45: Clarify the return value in the docstringThe
get_urimethod may returnNoneif the resource file is not found. Please update the@returnsection of the docstring to reflect this possibility.Suggested change:
@param framework: String GUI framework to get resources for (currently only 'qt5') -@return: Absolute path to the requested resource +@return: Absolute path to the requested resource or None if not found
47-47: Useos.path.joinfor path constructionTo ensure platform-independent path handling, consider using
os.path.joinwhen constructing file paths.Suggested change:
-path = f"{GUI_CACHE_PATH}/{self.res_namespace}/{framework}/{res_filename}" +path = join(GUI_CACHE_PATH, self.res_namespace, framework, res_filename)ovos_gui/namespace.py (1)
843-844: Avoid overwriting themessagevariable to prevent confusionReassigning the
messagevariable may cause confusion and hinder readability. It's better to use a different variable name to hold the forwarded message.Apply this diff to use a different variable name:
LOG.info(f"GUI with ID {gui_id} connected to core message bus") websocket_config = get_gui_websocket_config() port = websocket_config["base_port"] - message = message.forward("mycroft.gui.port", - dict(port=port, gui_id=gui_id, framework=framework)) - self.core_bus.emit(message) + forward_message = message.forward("mycroft.gui.port", + dict(port=port, gui_id=gui_id, framework=framework)) + self.core_bus.emit(forward_message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- README.md (2 hunks)
- ovos_gui/bus.py (2 hunks)
- ovos_gui/constants.py (1 hunks)
- ovos_gui/gui_file_server.py (0 hunks)
- ovos_gui/namespace.py (10 hunks)
- ovos_gui/page.py (2 hunks)
- protocol.md (1 hunks)
- test/unittests/test_bus.py (4 hunks)
- test/unittests/test_gui_file_server.py (0 hunks)
- test/unittests/test_namespace.py (8 hunks)
- test/unittests/test_page.py (0 hunks)
💤 Files with no reviewable changes (3)
- ovos_gui/gui_file_server.py
- test/unittests/test_gui_file_server.py
- test/unittests/test_page.py
✅ Files skipped from review due to trivial changes (1)
- ovos_gui/constants.py
🧰 Additional context used
🪛 LanguageTool
README.md
[typographical] ~63-~63: Consider adding a comma here.
Context: ...o be handled by client apps In case of containers a shared volume should be mounted betwe...(IN_CASE_OF_COMMA)
protocol.md
[uncategorized] ~64-~64: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...ight still exist client side, it is the clients responsibility to handle the namespace/...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🪛 Markdownlint
README.md
49-49: null
Bare URL used(MD034, no-bare-urls)
50-50: null
Bare URL used(MD034, no-bare-urls)
51-51: null
Bare URL used(MD034, no-bare-urls)
52-52: null
Bare URL used(MD034, no-bare-urls)
🪛 Ruff
ovos_gui/namespace.py
920-920: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (15)
README.md (2)
29-29: LGTM: Informative addition about QT5 clients.This note provides valuable information for users about the current state of client support.
55-60: LGTM: Informative addition of "Limitations" section.This new section clearly explains the expectations and limitations regarding GUI resource files and their accessibility. It provides valuable information for both users and developers, aligning well with the changes in resource management mentioned in the PR objectives.
test/unittests/test_bus.py (2)
3-3: LGTM: Import of List for type hinting.The addition of
from typing import Listis a good practice. It suggests the use of type hinting, which improves code readability and helps catch type-related errors early.
105-107: LGTM: Updated test case for client pages retrieval.The changes to
test_get_client_pagescorrectly reflect the new implementation using the_get_client_pagesmethod. The assertions now properly verify that the correct framework is used when retrieving page URIs.ovos_gui/bus.py (1)
250-251: LGTM! Consistent comments improve code clarity.The addition of these comments in the
send_gui_pagesmethod provides consistency with thesynchronizemethod and improves overall code clarity.test/unittests/test_namespace.py (7)
25-25: LGTM: New import for GUI_CACHE_PATH constant.This import aligns with the PR objectives and is likely used in the updated resource caching logic.
133-135: LGTM: Consistent removal ofurlparameter from GuiPage instantiations.These changes are in line with the modifications observed earlier and maintain consistency throughout the test cases.
150-152: LGTM: Consistent removal ofurlparameter from GuiPage instantiations.These changes maintain consistency with the earlier modifications, ensuring uniform treatment of
GuiPageobjects throughout the test suite.
175-177: LGTM: Consistent removal ofurlparameter from GuiPage instantiations.The changes in this method align with the previous modifications, maintaining a consistent approach to
GuiPageobject creation throughout the test cases.
Line range hint
1-444: Overall assessment: Changes align with PR objectives and improve test consistency.The modifications to this test file are consistent with the PR's goal of streamlining GUI resource management and addressing performance issues. Key improvements include:
- Removal of the
urlparameter fromGuiPageinstantiations, simplifying object creation.- Updates to message data structures, reflecting changes in how page identifiers are referenced.
- Refactoring of the system resource caching test to use a more standardized approach.
While these changes are generally positive, consider the suggestions for improving test isolation, particularly in the
test_upload_system_resourcesmethod. This will enhance the robustness and reliability of the test suite.
261-261: LGTM: Updated message data structure.The change from "page" to "page_names" aligns with the PR's goal of improving GUI resource management.
Please ensure that this change is consistent with the corresponding implementation in the
NamespaceManagerclass. Run the following script to verify:✅ Verification successful
Verified: NamespaceManager class has been updated to use 'page_names' accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify NamespaceManager class changes # Expected: Occurrences of 'page_names' instead of 'page' in relevant methods rg -p 'def handle_delete_page' ovos_gui rg -p 'page_names' ovos_guiLength of output: 434
68-72: LGTM: Removedurlparameter from GuiPage instantiations.This change simplifies the
GuiPagecreation process and aligns with the PR's goal of streamlining GUI resource management.Please ensure that these changes are consistent with modifications made to the
GuiPageclass. Run the following script to verify:✅ Verification successful
Verified:
urlparameter has been successfully removed from all GuiPage instantiations.No occurrences of the
urlparameter were found in anyGuiPageinstances across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify GuiPage class changes # Expected: No occurrences of 'url' parameter in GuiPage instantiations rg -p 'GuiPage\(' ovos_guiLength of output: 137
Script:
#!/bin/bash # Description: Check for any GuiPage instantiations with 'url' parameter # Expected: No occurrences of 'GuiPage(' with 'url=' rg 'GuiPage\([^)]*url=' ovos_guiLength of output: 34
ovos_gui/page.py (2)
36-38: Potential issue whennamespaceisNone
47-47: Resolved GUI pages limited to local filesovos_gui/namespace.py (1)
446-446:⚠️ Potential issueRemove unused variable
configin_init_gui_file_sharemethodThe variable
configis assigned but never used, which can be removed to clean up the code.Apply this diff to remove the unused variable:
def _init_gui_file_share(self): - config = Configuration().get("gui", {}) self._cache_system_resources()Likely invalid or redundant comment.
get rid of CPU spikes, OOM errors, it was a bad approach we tried!
companion to OpenVoiceOS/ovos-bus-client#120
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
New Features
GUI_CACHE_PATHfor managing cached GUI resources.Improvements
get_urimethod by removing unnecessary parameters and focusing on new resource resolution strategies.NamespaceandGuiPageto reflect recent changes in handling page identifiers.Bug Fixes
Chores
GuiPageclass.