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

Feature/ssp-pluggable-algorithms #448

Closed
wants to merge 10 commits into from

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Oct 30, 2019

This PR adds a plugin based system for parsing algorithms, modeled after the model_uri_resolver.

Since the load_ssp function needs yet another paramater for this to work, I decided to wrap this function in a statefull class named ssp_loader. This greatly simplifies usage.

cse::ssp_loader loader;
ssp_loader.set_override_algorithm = ...
ssp_loader.load(xmlPath);

Closes #437
Blocked by #447

@kyllingstad
Copy link
Member

I like the general idea of this. I'm just worried that it's a bit premature. It adds a lot of machinery to enable pluggable algorithms, when the fact of the matter is that we only have a single algorithm to plug in yet, and it's built into the cse-core library.

@markaren
Copy link
Contributor Author

markaren commented Oct 31, 2019

It adds a lot of machinery

I wouldn't say a lot. The ssp_loader class should be added regardless and can therefore be overlooked in this respect. I could even make yet another PR.

when the fact of the matter is that we only have a single algorithm to plug in yet, and it's built into the cse-core library.

I'm just worried that it's a bit premature

I think you agree that both the SSP parser and cse-config parsers must have this feature. We can rather discuss how it should be implemented than saying we do not need it right now.

A core issue with the current state of the SSP and CSE-config setup is that they are not fit for the future. Meaning they are bound to introduce breaking changes in the future. My recent PRs try to remedy this. At least for the SSP one. Then the CSE-config must catch up somehow.

@kyllingstad
Copy link
Member

I think you agree that both the SSP parser and cse-config parsers must have this feature. We can rather discuss how it should be implemented than saying we do not need it right now.

Sorry, I didn't mean to come across as negative. It's just that this comes without any prior discussion of either requirements or design (at least that I can remember), so these discussions must be had here and now. And in my mind, requirements must come before design.

Your design is nice and the code looks good. My point was that we don't know the requirements yet.

A core issue with the current state of the SSP and CSE-config setup is that they are not fit for the future. Meaning they are bound to introduce breaking changes in the future. My recent PRs try to remedy this. At least for the SSP one. Then the CSE-config must catch up somehow.

This does not prevent any breaking changes in the future. What if we decide to encapsulate algorithms in some FMU-like container (or just plain DLLs)? Then we need a URI (or at least file path) resolver, not a name resolver, and the XML structure in #447 would look very different.

As long as we hard-code the algorithm name in the XML schema, we might as well hard-code it in the C++ code.

I am fully aware that I tend to overthink things and let perfect stand in the way of good, though, and maybe I'm doing that here. You should probably wait for comments from the others and not listen too much to me. ;)

@markaren
Copy link
Contributor Author

It's just that this comes without any prior discussion of either requirements or design

True, although the requirement has always been for core to be plugin-based. But I agree this does come out of the blue.

As long as we hard-code the algorithm name in the XML schema

This PR does not use Algorithms names in the schema. Though it defines a schema for the FixedStepAlgorithm.

const std::string& algorithmName,
const boost::property_tree::ptree& tree)
{
if (algorithmName == "osp:FixedStepAlgorithm") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this correspond to an element name in the XML schema in #447?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is that the xsd does not specify any algorithms.

 <xs:element name="Algorithm">
        <xs:complexType>
            <xs:sequence>
                <xs:any processContents="skip"/>
            </xs:sequence>
        </xs:complexType>
    </xs:element>

@markaren
Copy link
Contributor Author

But we can put this on hold for a little while. Whats most important for me is to get #447 merged/approved. Been waiting a looong time for that.

@markaren markaren added the discussion needed Let's have a discussion about this label Nov 1, 2019
@markaren markaren changed the base branch from refactor/ssp-move-algo-to-defaultexperiment to master November 5, 2019 13:14
@markaren markaren changed the base branch from master to feature/ssp-loader-class November 5, 2019 13:15
@markaren markaren changed the base branch from feature/ssp-loader-class to master November 5, 2019 13:15
@markaren
Copy link
Contributor Author

markaren commented Nov 5, 2019

Closing for the time being. Better revisit in the future.

@markaren markaren closed this Nov 5, 2019
@markaren markaren deleted the feature/ssp-pluggable-algorithms branch November 7, 2019 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Let's have a discussion about this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SSP] Support any algorithm
2 participants