-
Notifications
You must be signed in to change notification settings - Fork 317
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
A new way of adding parameters that allows overriding inherited parameters (and sphinx auto documentation) #3098
base: main
Are you sure you want to change the base?
Conversation
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.
looks good to me!
Add tests
I suggest to create a module inside qcodes/instrument_drivers (next to test.py and device.py) with a few example instruments (for example, those from the PR description) that use this new feature. They can also be used later to apply the sphinx hacks and render as part of qcodes docs
After this MRs one with docs on how to "hack" Sphinx in order to document these parameters automatically.
do you mean that you'd like to land the "new way of adding parameters" first, and then make another PR that would document/implement the sphinx hacks, right?
I have another separate question. so this feature enables 1) override existing parameters in instrument driver subclasses such that the original parameter won't need even be instantiated, and 2) produce sphinx autodoc documentation for parameters defined in this new way. do you see a lot of widespread use of the capability 1)? i am not so sure about it, because it really depends on how the driver depends on that parameter that one wants to override - for example, if a parameter is used in the init logic of the driver in some way unreplaceable, then overriding may be tricky, and it might be important to keep the original parameter of the parent class so it gets added and used, and the in the subclass one can substitute it with a modified one by doing if you agree with me on this, then the capability 2) in my perspective becomes more widespread, hence i'd suggest to add all code and documentation related to in in this same PR :) what do you think? |
@astafan8 thank you very much for quick review, comments have been addressed.
It was the capability 1) that triggered me to dive into this. Not sure how widespread it will be, but has certainly important applications. The scenario that I believe was most relevant is for virtual instruments that serve as (snapshotable) information container, e.g.,
Fair point, there might some complicated edge cases where this solution is not flexible enough, and perhaps it will never be able to cover every possible case, though it would be nice to be able to use this approach and make sphinx work for all or most scenarios. I am happy to make it more versatile to accommodate for a wide range of use cases, though it would be better to have an exemplary case before building a solution for non-yet-existing problem. Maybe tagging me in future issues/PRs related to this can help mature this approach as people start using it. A related potential problem that pops up in my mind is that the order in which parameters are added might matter as well. And if that is the case then the complexity of the inheritance/overriding scheme might grow.
I am up for this. Changed the description and todo list. For now I think I will wait to gather some more feedback on the implementation and the issues you raised. The solution for these potential problems is not very clear for now. |
agree. happy to hear more about the
indeed.
great! then i'll review after those items are done, ok? |
Yes, deal! I will tag you when it is ready for a new review :) |
Thank you for making this ❤️ , you know how much I have been looking forward to this. I'll try to focus on the "user" side of how this change would affect the writing and readability of instrument drivers as well as the resulting docstrings and how easy they are to understand. I'll try to minimize my comments on the implementation details as the sphinx+decorator magic is a bit of well, magic, to me. Example instrument "new-style"
Issue #1151 contains a nice example of how an instrument driver looks in the new style. I assume there is probably some subtle differences between the initial design and the current implementation. Could you add an example driver to this MR and maybe also include the sphinx hack so that it is rendered in the docs? Maybe the best example would be to rewrite the SGS100A microwave source driver using this new style. The reason is that this is the simplest driver, and it would serve as an example on how to write drivers "new-style". However, I see two issues with this. 1. it would delay/block the acceptance of this MR because the new driver needs to be tested, and 2. it enforces a standard before it is tested in working nice. So we should probably postpone the replacing to a later date. Tweaks to implementation of instrument "new-style"I'll leave some comments here on the example from #1151. from qcodes import Instrument, ManualParameter, Parameter
from qcodes import validators
class MyInstrumentDriver(ModifiedInstrumentClass):
"""
MyInstrumentDriver docstring
"""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self._freq = None
@add_parameter
def _parameter_time( # NB the parameter name will be just `time`
self,
parameter_class=ManualParameter,
initial_value=3,
unit="s",
label="Time",
vals=validators.Numbers(),
):
"""Docstring of :code:`time` parameter""" suggestion, the @add_parameter decorator in combination with the Other than that, I really like the syntax. I've also left a comment in the @add_parameter
def _parameter_freq( # NB the parameter name will be just `freq`
self,
parameter_class=Parameter,
unit="Hz",
label="Frequency",
vals=validators.Numbers(),
set_cmd="_set_freq", # `self._set_freq` will be passed to `Parameter`
get_cmd="_get_freq", # `self._get_freq` will be passed to `Parameter`
):
"""Docstring of :code:`freq` parameter""" def _set_freq(self, value):
self._freq = value
def _get_freq(self):
return self._freq Good that you included these in the example. praise in general, I really like this way of writing drivers from a usability perspective. To me it seems very much in line with the original philosophy of QCoDeS of being easy to write and read for physicists (which does require hiding the magic, but that is fine), but I'll leave that to @astafan8 as the official QCoDeS representative in this MR. Feedback on how docs are renderedbased on the screenshot above. praiseI think the fact that parameters now show up in the docs is a vast improvement over not being able to document them at all 👍 I have some questions as I am not as familiar with the sphinx magic you had to invoke to get this to work. At first sight, the parameter documentation seems a bit off and hard to read. This is because all is crammed into the call signature, and only the docstring shows. A big improvement (in my opinion) is that now you don't get the massive docstring of the parameter base class when. This used to bloat the readability of individual parameters for the user. I had a look at some of our quantify docs (see screenshot) to see how it works for "normal" objects. Unless it is trivially easy, I suggest you do not implement these things, I'm mostly trying to assess if this would be possible in the future. |
agree with @AdriaanRol (especially happy for the point 2) - let's not add a driver of a real instrument in this PR, just stick to a dummy/test kind of driver. |
Thank you for the review @AdriaanRol Will have to process farther some parts of your feedback, here are a few replies.
definitely agree, it is a technical limitation in python, if a method
Agreed, though will likely keep it out of scope for this Pull Request.
Very likely, sphinx magic and hacking can go pretty far, doing it the "intended" way might be harder.
Should be.
Should be possible to automate this, and I think should be automated, a heads up for making it compatible with both numpy and google style docstring. suggestion if the second is preferred, let's include that in the example docstring (i.e., what the settable is, a link to the set_cmd and get_cmd, the unit, etc. User will have this freedom anyway, but the idea would be to automate as much as possible. Note for selfSome Inspiration: from sphinx.ext import autodoc
autodoc.PropertyDocumenter?? Init signature:
autodoc.PropertyDocumenter(
directive: 'DocumenterBridge',
name: str,
indent: str = '',
) -> None
Source:
class PropertyDocumenter(DocstringStripSignatureMixin, ClassLevelDocumenter): # type: ignore
"""
Specialized Documenter subclass for properties.
"""
objtype = 'property'
directivetype = 'method'
member_order = 60
# before AttributeDocumenter
priority = AttributeDocumenter.priority + 1
@classmethod
def can_document_member(cls, member: Any, membername: str, isattr: bool, parent: Any
) -> bool:
return inspect.isproperty(member) and isinstance(parent, ClassDocumenter)
def document_members(self, all_members: bool = False) -> None:
pass
def get_real_modname(self) -> str:
return self.get_attr(self.parent or self.object, '__module__', None) \
or self.modname
def add_directive_header(self, sig: str) -> None:
super().add_directive_header(sig)
sourcename = self.get_sourcename()
if inspect.isabstractmethod(self.object):
self.add_line(' :abstractmethod:', sourcename)
self.add_line(' :property:', sourcename)
File: /usr/local/anaconda3/envs/q38/lib/python3.8/site-packages/sphinx/ext/autodoc/__init__.py
Type: type
Subclasses: Note the Internally the Documenter seems to create something like: .. py:method:: MyInstrumentDriver.bla
:module: quantify.utilities.my_mod
:property:
My docstring over here Above A "role" in sphinx can be added with def parameter_role(name, rawtext, text, lineno ,inliner):
# ... ???
# Somehow make it an alias of the default `:meth:` role
def setup(app):
app.add_role("parameter", parameter_role) A custom documenter as part of qcodes sphinx extension might be the proper way of doing this. |
Latest prototype, everything auto-generated: Sphinx hacks are more elegant than I was imagining. conf.py source codefrom sphinx.ext import autodoc
from sphinx.domains import python as sdp
QCODES_ADD_PARAMETER_FLAG = "_add_parameter"
QCODES_METHOD_PREFIX = "_parameter_"
def format_name(self) -> str:
if hasattr(self.object, QCODES_ADD_PARAMETER_FLAG):
return ".".join(
self.objpath[:-1] + [self.objpath[-1][len(QCODES_METHOD_PREFIX) :]]
)
return ".".join(self.objpath) or self.modname
original_add_directive_header = autodoc.MethodDocumenter.add_directive_header
def add_directive_header(self, sig: str) -> None:
original_add_directive_header(self, sig)
if self.object_name.startswith("_parameter_"):
sourcename = self.get_sourcename()
self.add_line(" :parameter:", sourcename)
original_get_signature_prefix = sdp.PyMethod.get_signature_prefix
def get_signature_prefix(self, sig: str) -> str:
prefix_str = original_get_signature_prefix(self, sig)
if "parameter" in self.options:
prefix_str += "parameter "
return prefix_str
def dont_skip_qcodes_params(app, what, name, obj, skip, options) -> bool:
if hasattr(obj, QCODES_ADD_PARAMETER_FLAG):
return False
return skip
import inspect
def add_parameter_spec_to_docstring(app, what, name, obj, options, lines):
# name is e.g. `"my_module._parameter_time"
if QCODES_METHOD_PREFIX in name:
lines += [
"",
".. rubric:: Arguments passed to "
":meth:`~qcodes.instrument.base.InstrumentBase.add_parameter`:",
"",
]
for kw_name, par_obj in inspect.signature(obj).parameters.items():
if kw_name != "self":
if kw_name == "parameter_class":
# create link to the parameter class
mod = par_obj.default.__module__
class_name = par_obj.default.__name__
value = f":class:`~{mod}.{class_name}`"
else:
value = f"*{par_obj.default!r}*"
if kw_name == "vals":
kw_name = ":mod:`vals <qcodes.utils.validators>`"
else:
kw_name = f"**{kw_name}**"
lines.append(f"- {kw_name} = {value}")
lines += [""]
return lines
def clear_parameter_method_signature(
app, what, name, obj, options, signature, return_annotation
):
if QCODES_METHOD_PREFIX in name:
signature = "()"
return (signature, return_annotation)
def setup(app):
# we can't have a method in the class with the same name as the desired
# parameter, therefore we patch the method name displayed in the sphinx docs
# monkey patching `MethodDocumenter`
# e.g. `_parameter_time` -> `_parameter_time` when displayed in the docs
autodoc.MethodDocumenter.format_name = format_name
autodoc.MethodDocumenter.add_directive_header = add_directive_header
sdp.PyMethod.option_spec.update(
{"parameter": sdp.PyMethod.option_spec["classmethod"]}
)
sdp.PyMethod.get_signature_prefix = get_signature_prefix
# enforce documenting the decorated private method
app.connect("autodoc-skip-member", dont_skip_qcodes_params)
app.connect("autodoc-process-docstring", add_parameter_spec_to_docstring)
app.connect("autodoc-process-signature", clear_parameter_method_signature) Good night! 😝 |
…to feat/add_parameter_decorator
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.
the implementation looks reasonable! most of the comments are about semantics, and clarity in the documentation :) for the rest, looks like the PR will very soon be ready for merge!
…to feat/add_parameter_decorator
…to feat/add_parameter_decorator
@astafan8 I think I have addressed all comment regarding the docs. Will continue later this week (hopefully) with some tests. |
@astafan8 tests added! It would be nice to have them for the sphinx extension as well, but I have no idea how that is done nor how difficult or time consuming it is to do it properly, sphinx in general is a bit of a magic black box. Chores: You need to configure codacy to ignore either D212 or D213 they are mutually excluding otherwise the pipeline will never pass... Could you take care of these warnings according to the standard that you adopt? Ready to merge? :) |
Hey folks! Any progress on this PR? let me know if there is anything required from my side :) |
@caenrigen Sorry for waiting. Your work on this feature initiated a thorough alignment among maintainers, and now we are coming back with a suggestion. It started with the observation that while what this PR (the private method approach) enables is very neat, the code one the driver developer side is a bit too complex making the drivers somewhat harder to read. We were looking into options of how to preserve the simple/straightforward syntax of defining parameters and still have the benefit of documenting parameters. So, I tried coming up with a suggestion that makes driver implementation simpler but at the cost of moving the logic to enable the documentation into a sphinx extension that has to rely on parsing the code. I pushed the alternative suggestion into #3188. What do you think of it? (we can discuss the tech details of the proposal in that PR) |
Hi! @jenshnielsen Thank you for the updated, was reading the description of #3188 just before your comment.
I am glad that you have looked into this more deeply 🙂. I am taking a few days off and will try to have a look into #3188 somewhere by the end of next week. For now I will leave a few comments.
I agree, it is a bit more complex, it abuses python in a few ways 😅. We found the benefits worth it since no alternative was available, and I put significant effort into documenting how it is intended to be used. Writing a qcodes driver requires reading docs in most cases anyway, I had to do it myself when writing QCoDeS/Qcodes_contrib_drivers#98. I would suggest to have a look there to confirm that #3188 can provide the same functionalities, I used the In short, I am open to alternatives as long as their simplicity still allows for complex functionality. "Simple things should be simple, complex things should be possible."
(Disclaimer: I have not look at the implementation) My intuition is that parsing code can be more fragile. Also, I had a hard time creating the sphinx extension, therefore I am not sure if moving a lot of complexity in there is a good idea, since it might be very hard to maintain. I will try to gather some feedback within Quantify, and play with #3188 when I am back so that I can have an informed opinion on this topic. One last question: Do you see both approaches possible to co-exist in qcodes? so that developers can choose themselves? PS thank you for the #3188! From my own experience, I can imagine that takes some significant work. |
@caenrigen thanks for your comments
Technically I don't think there are any reasons why they cannot. I would say that if there are significant advantages from this approach that the other one does not give we can add this one too. Given the work on abstract parameters in #3125 I see that problem as solved without any more work but perhaps I miss something in this pr? That means that both these prs are largely concerned with improving the documentation.
This is largely a matter of preferences but I personally tend to prefer idiomatic python style not only for writing code but also for reading (both for humans and for tools). For example if you implement the
I do agree that is not ideal however I see the trade off a bit differently. To me it is more important that the driver code is as clean as possible and the more complex code is unused at runtime.
I was able to take significant advantage from your work on how to setup a sphinx extension so thanks for that. It saved me a significant amount of time :) |
Hi guys! Due to limitations in my bandwidth I won't be able to have a proper look into #3188 until likely somewhere in August.
@jenshnielsen considering that #3188 might take a while to fully develop, and that this PR is ready and in principle not conflicting with #3188, any chance of including it in 0.27.0? The #3188 is still interesting and I will be glad to help with that later :) |
Thanks again for your contribution. I know you have put a lot of time and effort into this thanks to your contribution, the issue was brought to our attention and we were able to think of ways to do it, but unfortunately we have come to the conclusion that this design is too complicated for us to be able to commit to maintaining it unless there is a significant further gain from this method. In addition we are a bit concerned that 3 methods for adding parameters to instruments is too much and will result in confusion among users. The part of #3188 that is concerned with changes to the actual code is very close to landing via #3191 and has basically only been held back due to me wanting to let it bake on master for awhile before releasing. It will therefor target 0.28. Whit that change the existence of parameters can be documented as long as they are documented as any other attribute. With regards to the parsing of attributes to improve the documentation of them I feel like this is something that we could actually generalise outside QCoDeS. Rather than the code I originally wrote to remove self parameters and instantiate the parameters we can generate a proxy parameter with a repr that contains the full syntax. That will be a lot simpler and easier to reason about and not specific to QCoDeS parameters. |
@jenshnielsen thank you for the reply and consideration
fair enough
Looking forward to this, sounds interesting, keep me updated on the developments of this, feel free to tag me in related PRs. (I also joined the QCoDeS Slack, we can chat there as well when needed.) Cheers |
This is a follow up from the discussion in #1151, marked as Draft for now to gather feedback.
Fixes #32
Also sets the infrastructure for fixing #1151
Changes proposed in this pull request:
Parameters
inheritance with overriding capabilities (Overwriting parameters in inherited instruments #32).Sphinx result + example, please note the
InstanceAttr
in the signature of the added parameters (and source code):Source code
To do
del
it later and replace it with a new one from the child class?After this MRs one with docs on how to "hack" Sphinx in order to document these parameters automatically.@astafan8 here is a Draft implementation.
@AdriaanRol @JordyGloudemans your feedback is welcome as well.