Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

Config loaders #1116

Draft
wants to merge 104 commits into
base: develop
Choose a base branch
from
Draft

Conversation

stanislaw47
Copy link

@stanislaw47 stanislaw47 commented Jun 13, 2020

Hello Taurus community
This PR provides what I wanted in Taurus for long time: ability for load configuration for TaurusGUI from JSON.

I created abstraction for config source, be it a Python module or XML/JSON file. In that way, adding support for any other config file format, like TOML or YAML should be trivial. TaurusGUI now loads configuration by accessing fields of *ConfigLoader class. Whole interface of *ConfigLoader class is defined in AbstractConfigLoader by using Python abc module.

All discovery logic happens in taurus.qt.qtgui.taurusgui.config_loader.getLoader function. It matches loaders by file extensions. In case of directory or non-existing path, it tries to use PyConfigLoader. In case of using entry-points, logic has to be moved elsewhere but for the moment I do not know how to achieve it, especially when taking into account possibility to pass directory and Python module path.

Regarding abc.ABC class for Python 2 and 3 - I know that Taurus uses future package for compatibility. However, I did not found anything there about providing abc.ABC so I created my own compatibility from what I found on StackOverflow.

Things to do:

  • test for loading configuration from different sources
  • backward compatibility tests
  • entry-point for registering custom config loaders

@cpascual
Copy link
Member

Hi @stanislaw47 Thanks a lot for this contribution!, here are a few preliminary comments:

  • I really like the idea, and I am happy because it comes to clean quite a messy part of Taurus (the config loading)
  • Unless I missed something (I gave a quick glance only) , I thing this PR still misses the part in which the getLoader is called, and then the returner loaded used, right?
  • I agree in that getLoader should be implemented with entry-points. I think it is relatively easy (I've been doing some work in preparation of TEP13 lately and I can help with it. I suggest to use the following entry point name: taurus.gui.loaders and iterate over it in getLoader. Also, the *ConfigLoader itself can declare its supported extension(s) (and we treat the pyconf case as an special fallback case)
  • As for the abstract loader: I think this is a good opportunity to clean old/deprecated stuff. For example, the monitor stuff can be removed. Also, the sardana-related configs should not be implemented/required in taurus. A possible solution is to allow multiple config loaders for the same extension (e.g. if you provide a json file, all the loaders that declare themselves supporting json would be activated and used, and in this way taurus would declare a json loader that handles onlyy taurus configs while sardana would declare a json loader that handles the sardana-related configs, and both would be used). Note that in order to completely isolate taurus and sardana configuration handling, the TaurusGui.loadConfiguration() will also need to be refactored to use plugins, but this can be done in two steps.
  • Regarding python2 compatibility: I do not expect to maintain py2 support in taurus after the Jul20 release, so I personally would not struggle with making the new feature py2-compatible. If it is just a matter of the small compatibility hack with abc, then it is fine, but if it requires more work, I would make this a py3-only feature and fall back to the old mechanism for py2 if we integrate this PR before Jul20. The same for Qt4.
  • Regarding tests: I just did a large refactoring of the tests in Fix test suite #1114 . As soon as it is merged, you should update this branch with it. Then I would use pytest to do unit tests for the loaders and for the backwards-compatibility
  • Do you have any plans for a "*ConfigWriter?". At the moment, both the "new gui wizard" and the TaurusGui themselves are able to create xml config files.

@stanislaw47
Copy link
Author

Hi @cpascual

I really like the idea, and I am happy because it comes to clean quite a messy part of Taurus (the config loading)

I'm happy to hear it

Unless I missed something (I gave a quick glance only) , I thing this PR still misses the part in which the getLoader is called, and then the returner loaded used, right?

Actually, it's there, in here. Resulting conf object is instance of *ConfigLoader nad is passed to specific loading methods.

I agree in that getLoader should be implemented with entry-points. I think it is relatively easy (I've been doing some work in preparation of TEP13 lately and I can help with it. I suggest to use the following entry point name: taurus.gui.loaders and iterate over it in getLoader. Also, the *ConfigLoader itself can declare its supported extension(s) (and we treat the pyconf case as an special fallback case)

I got trouble getting my head around pyconf since it support noto only extension but also directories and non-exisitng paths. But if it'll be treated as special fallback case, then I would be trivial indeed. I'll try to implement it soon.

As for the abstract loader: I think this is a good opportunity to clean old/deprecated stuff. For example, the monitor stuff can be removed. Also, the sardana-related configs should not be implemented/required in taurus. A possible solution is to allow multiple config loaders for the same extension (e.g. if you provide a json file, all the loaders that declare themselves supporting json would be activated and used, and in this way taurus would declare a json loader that handles onlyy taurus configs while sardana would declare a json loader that handles the sardana-related configs, and both would be used). Note that in order to completely isolate taurus and sardana configuration handling, the TaurusGui.loadConfiguration() will also need to be refactored to use plugins, but this can be done in two steps.

I am happy to remove deprecated stuff like MONITOR or CONSOLE, I kept them for backward compatibility. About multiple loaders for same file extensions - I actually do not know how that would work with entrypoints. Would one plugin override the other? Or would it allow them to work at the same time? Could you describe it a little more?

Regarding python2 compatibility: I do not expect to maintain py2 support in taurus after the Jul20 release, so I personally would not struggle with making the new feature py2-compatible. If it is just a matter of the small compatibility hack with abc, then it is fine, but if it requires more work, I would make this a py3-only feature and fall back to the old mechanism for py2 if we integrate this PR before Jul20. The same for Qt4.

For now, only compatibility is indeed ABC hack so I'll just keep it compatible for both Python 2 and 3. I believe this PR does not require special care for PyQt version, at least for now.

Regarding tests: I just did a large refactoring of the tests in #1114 . As soon as it is merged, you should update this branch with it. Then I would use pytest to do unit tests for the loaders and for the backwards-compatibility

Sure think, will do. I've written some tests using pytest so that should not be a problem.

Do you have any plans for a "*ConfigWriter?". At the moment, both the "new gui wizard" and the TaurusGui themselves are able to create xml config files.

Dumping configuration in given format would be cool feature. However, I think that might be non-trivial for py files. And could make this PR much bigger. Regarding plugins - should they support both loading and dumping configuration? Or just loading should be mandatory and dumping a feature? I personally would go for both loading and dumping as mandatory.

@stanislaw47
Copy link
Author

Hi @cpascual
sorry for the long break.
I've updated my branch with new approach to special config values (CONSOLE i.e.). In general, there should be generic representation of config source( be it Python module, xml or json file) and yet another entrypoint (taurus.config.properties). Functions registered using this entrypoint receive TaurusGUI instance. This allows very flexible behavior nad loading extra stuff i.e. Sardana related.
I've put SYNOPTIC into separate module because for me it seems like extra thing. I can revert it if this is necessary.

I would appreciate review to validate that this approach is good enough.

@cpascual
Copy link
Member

cpascual commented Feb 9, 2021

ok... give me a bit of time (I've my hands full adapting the testsuite so that it can be run with gitlab-ci). But do not hesitate to ping if I forget

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants