Implement get_library_information interface for Remote libraries (Rob… #3802
Conversation
…ot side) Prevents excessive load times for large remote libraries, by loading all keyword information in a single go.
Codecov Report
@@ Coverage Diff @@
## master #3802 +/- ##
==========================================
- Coverage 75.26% 75.21% -0.04%
==========================================
Files 221 221
Lines 18704 18704
Branches 3037 3037
==========================================
- Hits 14075 14066 -9
- Misses 4094 4100 +6
- Partials 535 538 +3
Continue to review full report at Codecov.
|
|
My initial statement about there being no documentation impact was false. That was only true for the user part. Documention has now been extended in the remote protocol section. |
| def get_library_information(self): | ||
| info_dict = dict() | ||
| for kw in self.get_keyword_names(): | ||
| kw_dict = dict() |
There was a problem hiding this comment.
what about just:
info_dict[kw] = {
'args': self.get_keyword_arguments(kw),
'tags': self.get_keyword_tags(kw)
'doc': self.get_keyword_documentation(kw)
}
There was a problem hiding this comment.
Hi @bhirsz and thanks for reviewing!
Good suggestion, I might even prefer:
info_dict[kw] = dict(args=self.get_keyword_arguments(kw),
tags=self.get_keyword_tags(kw),
doc=self.get_keyword_documentation(kw))
I have similar code in the Python remote server implementation. I will update there as well.
src/robot/libraries/Remote.py
Outdated
| try: | ||
| with self._server as server: | ||
| self.__kw_cache = server.get_library_information() | ||
| except: |
There was a problem hiding this comment.
Too broad exception, what are usual expected exceptions here? (in case of not supported version etc)
There was a problem hiding this comment.
I figured there were 2 possible ways to go here.
- Check if get_library_information is implemented. If so, use it and let it fail whenever it does.
- Just try to call it and fall back to the old way if it doesn't work, for whatever reason.
I went for the latter , because I thought it stayed closest to the existing behaviour. Not a fan of broad exception handling myself, so we could still opt for the first option.
There was a problem hiding this comment.
Introspection isn't available, so checking if it is implemented can't be done. But as it turns out the exceptions were already narrowed down on another level, that also contained a retry. Now the try on get_library_info replaces the first attempt and only the specific error is caught.
src/robot/libraries/Remote.py
Outdated
| def get_keyword_names(self): | ||
| if self._kw_cache: |
There was a problem hiding this comment.
Not sure what is possible with the Python version we need to support but I would try to implement decorator similiar to lru_cache instead, with optional argument. Ie:
@keyword_cache()
@keyword_cache(type='tags')
..
There was a problem hiding this comment.
I have now also processed this comment.
Processed review remarks and fixed an issue with the special cases for __intro__ and __init__ docstrings.
|
As I just commented issue #3362, it would be great to get this into RF 4.0. I looked at this PR now and it seems ok so that ought to be doable. The code is somewhat complex with all decorators and caching and I'm not sure is that needed. Wouldn't it be enough to call |
|
If you @JFoederer are around, we can discuss this further on the #devel channel on our Slack. Already started a thread about this enhancement there. |
Code simplification based on the information that get_keyword_names() is always called first and exactly once.
|
Thanks @JFoederer for updating the implementation to call I think we can simplify this a bit more by replacing the decorator with a helper method. In addition to that, probably need to enhance the documentation a bit. At least a note about this being new in RF 4.0 is needed. I'll do these changes myself after merging this PR. |
In preparation of resolving issue #3362 about load-time performance of large libraries, this pull request offers a forward and backward compatible interface extension as suggested in the issue.
With this change the Remote library will utilize the new
get_library_informationcall to retrieve all information in a single go. This significantly reduces load times of large libraries, from minutes to a couple of seconds. When the new call is unavailable or fails, the regular interface is used to provide both backward compatibility and maintain analyzability in case of failure.This pull request includes the change to Robot framework code (1 file), a new acceptance test (3 files) and a fix for the remote server (1 file) in the testdata. The latter could return None while None marshalling is disabled by default. There is no impact on documentation.
There appears to be some technical debt on the remote server side. Pull request #73 has been opened to first fix the current baseline of tests before pushing any updates to that side of the interface. Input requested.