Skip to content

Add full_product_spec and use that in provider API #501

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.

Sign up for GitHub

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

Closed
beojan wants to merge 1 commit into from
Closed

Add full_product_spec and use that in provider API #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions phlex/core/declared_provider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#include "phlex/core/declared_provider.hpp"
#include "phlex/model/full_product_spec.hpp"

namespace phlex::experimental {
declared_provider::declared_provider(algorithm_name name, product_query output_product) :
declared_provider::declared_provider(algorithm_name name, full_product_spec output_product) :
name_{std::move(name)}, output_product_{std::move(output_product)}
{
}
Expand All @@ -10,10 +11,10 @@

std::string declared_provider::full_name() const { return name_.full(); }

product_query const& declared_provider::output_product() const noexcept
full_product_spec const& declared_provider::output_product() const noexcept
{
return output_product_;
}

identifier const& declared_provider::layer() const noexcept { return output_product_.layer; }
identifier const& declared_provider::layer() const noexcept { return output_product_.layer(); }

Check warning on line 19 in phlex/core/declared_provider.cpp

View check run for this annotation

Codecov / codecov/patch

phlex/core/declared_provider.cpp#L19 

Added line #L19 was not covered by tests
}
13 changes: 6 additions & 7 deletions phlex/core/declared_provider.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "phlex/core/message.hpp"
#include "phlex/model/algorithm_name.hpp"
#include "phlex/model/data_cell_index.hpp"
#include "phlex/model/full_product_spec.hpp"
#include "phlex/model/product_specification.hpp"
#include "phlex/model/product_store.hpp"
#include "phlex/utilities/simple_ptr_map.hpp"
Expand All @@ -26,11 +27,11 @@ namespace phlex::experimental {

class PHLEX_CORE_EXPORT declared_provider {
public:
declared_provider(algorithm_name name, product_query output_product);
declared_provider(algorithm_name name, full_product_spec output_product);
virtual ~declared_provider();

std::string full_name() const;
product_query const& output_product() const noexcept;
full_product_spec const& output_product() const noexcept;
identifier const& layer() const noexcept;

virtual tbb::flow::receiver<index_message>* input_port() = 0;
Expand All @@ -39,7 +40,7 @@ namespace phlex::experimental {

private:
algorithm_name name_;
product_query output_product_;
full_product_spec output_product_;
};

using declared_provider_ptr = std::unique_ptr<declared_provider>;
Expand All @@ -56,11 +57,9 @@ namespace phlex::experimental {
std::size_t concurrency,
tbb::flow::graph& g,
AlgorithmBits alg,
product_query output) :
full_product_spec output) :
declared_provider{std::move(name), output},
output_{algorithm_name::create(std::string_view(identifier(output.creator))),
output.suffix.value_or(identifier("")),
output.type},
output_{output.spec()},
provider_{g,
concurrency,
[this, ft = alg.release_algorithm()](index_message const& index_msg, auto& output) {
Expand Down
16 changes: 16 additions & 0 deletions phlex/core/product_query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@
return true;
}

// Check if a full_product_spec satisfies this query
bool product_query::match(experimental::full_product_spec const& spec) const
{
using experimental::identifier;
if (!match(spec.spec())) {
return false;
}
if (identifier(layer) != spec.layer()) {
return false;
}
if (stage && stage != spec.stage()) {
return false;

Check warning on line 58 in phlex/core/product_query.cpp

View check run for this annotation

Codecov / codecov/patch

phlex/core/product_query.cpp#L58 

Added line #L58 was not covered by tests
}
return true;
}

std::string product_query::to_string() const
{
if (suffix) {
Expand Down
19 changes: 19 additions & 0 deletions phlex/core/product_query.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "phlex/phlex_core_export.hpp"

#include "phlex/model/full_product_spec.hpp"
#include "phlex/model/identifier.hpp"
#include "phlex/model/product_specification.hpp"
#include "phlex/model/product_store.hpp"
Expand Down Expand Up @@ -80,10 +81,28 @@ namespace phlex {
// Check if a product_specification satisfies this query
bool match(experimental::product_specification const& spec) const;

// Check if a full_product_spec satisfies this query
bool match(experimental::full_product_spec const& spec) const;

std::string to_string() const;

bool operator==(product_query const& rhs) const;
std::strong_ordering operator<=>(product_query const& rhs) const;

// Transitional automatic conversion operator so I don't have to rewrite all the tests
// The deprecated annotation is commented out because we have -Werror on
// [[deprecated(
// "Generation of a full_product_spec from a product_query is only transitionally supported")]]
Comment on lines +92 to +95
Copy link
Copy Markdown
Member

@knoepfel knoepfel Apr 15, 2026

Choose a reason for hiding this comment

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

The deprecation attribute is fine for our users. They decide how to build their own Phlex-dependent code, with or without -Werror and -Wno-error=deprecated-declarations.

For our own purposes, we'll continue building with -Werror and not exclude deprecated declarations. We should convert all tests to use the encouraged API. If we want to test the deprecation, we can add a dedicated compile-only test that verifies the deprecation message. I'm not sure that's necessary, though.

operator experimental::full_product_spec()
{
return experimental::full_product_spec{
experimental::product_specification{experimental::algorithm_name::create(std::string_view(
experimental::identifier(this->creator))),
this->suffix.value(),
this->type},
experimental::identifier(this->layer),
this->stage.value_or("")};
}
};

inline std::string format_as(product_query const& q) { return q.to_string(); }
Expand Down
4 changes: 2 additions & 2 deletions phlex/core/registration_api.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ namespace phlex::experimental {
{
}

auto output_product(product_query output)
auto output_product(full_product_spec output)
{
using return_type = return_type<typename AlgorithmBits::algorithm_type>;
using provider_type = provider_node<AlgorithmBits>;

output.type = make_type_id<return_type>();
output.set_type(make_type_id<return_type>());

registrar_.set_creator([this, output = std::move(output)](
auto /* predicates */, auto /* output_product_suffixes */) {
Expand Down
54 changes: 54 additions & 0 deletions phlex/model/full_product_spec.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#ifndef PHLEX_MODEL_FULL_PRODUCT_SPEC_HPP
#define PHLEX_MODEL_FULL_PRODUCT_SPEC_HPP

#include "phlex/phlex_model_export.hpp"

#include "phlex/model/identifier.hpp"
#include "phlex/model/product_specification.hpp"
#include "phlex/model/type_id.hpp"

#include "boost/container_hash/hash.hpp"
#include "fmt/format.h"

namespace phlex::experimental {
class PHLEX_MODEL_EXPORT full_product_spec {
public:
full_product_spec(product_specification spec, identifier layer, identifier stage) :
Copy link
Copy Markdown
Member

@knoepfel knoepfel Apr 15, 2026

Choose a reason for hiding this comment

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

I am leery of creating a full product specification when what is constructed is not, in fact, a full specification (the type is not set until later). We did this for product_query/product_specification, and I'd like to fix this...specifically, I'd like to avoid the need for setters wherever possible.

I think this means that the provider's output_product registration interface requires all of the data-product fields encapsulated by product_specification, layer, and stage, but maybe they can be received from the user in a more ergonomic way (e.g., require the arguments algorithm_name, suffix, layer, stage, where stage can default to an identifier that denotes the current process):

m.provide(...)
 .output_product(
   /* algorithm_name*/ creator,  
   /* identifier */ suffix, 
   /* identifier */ layer, 
   /* identifier, possibly defaulted */ stage
  )

How these fields are packaged is then an implementation detail.

Copy link
Copy Markdown
Contributor Author

@beojan beojan May 15, 2026

Choose a reason for hiding this comment

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

#602 takes this approach (drafted for now). I'll close this PR.

spec_{std::move(spec)}, layer_{std::move(layer)}, stage_{std::move(stage)}
{
}

bool operator==(full_product_spec const&) const noexcept = default;

product_specification const& spec() const noexcept { return spec_; }
algorithm_name const& creator() const noexcept { return spec_.qualifier(); }
identifier const& suffix() const noexcept { return spec_.suffix(); }
type_id type() const noexcept { return spec_.type(); }

Check warning on line 26 in phlex/model/full_product_spec.hpp

View check run for this annotation

Codecov / codecov/patch

phlex/model/full_product_spec.hpp#L26 

Added line #L26 was not covered by tests
void set_type(type_id&& type) { spec_.set_type(std::move(type)); }
identifier const& layer() const noexcept { return layer_; }
identifier const& stage() const noexcept { return stage_; }

Check warning on line 29 in phlex/model/full_product_spec.hpp

View check run for this annotation

Codecov / codecov/patch

phlex/model/full_product_spec.hpp#L29 

Added line #L29 was not covered by tests
std::size_t hash() const noexcept
{
std::size_t result = creator().plugin().hash();
boost::hash_combine(result, creator().algorithm().hash());
boost::hash_combine(result, suffix().hash());
boost::hash_combine(result, layer().hash());
boost::hash_combine(result, stage().hash());
boost::hash_combine(result, type());
return result;

Check warning on line 38 in phlex/model/full_product_spec.hpp

View check run for this annotation

Codecov / codecov/patch

phlex/model/full_product_spec.hpp#L32-L38 

Added lines #L32 - L38 were not covered by tests
}

std::string to_string() const
{
return fmt::format(
"{}:{}/{} ϵ {}", creator().plugin(), creator().algorithm(), suffix(), layer());
}

private:
product_specification spec_;
identifier layer_;
identifier stage_;
};
}

#endif // PHLEX_MODEL_FULL_PRODUCT_SPEC_HPP
33 changes: 20 additions & 13 deletions plugins/python/src/modulewrap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "wrap.hpp"

#include "phlex/model/data_cell_index.hpp"
#include "phlex/model/full_product_spec.hpp"

#include <fmt/format.h>
#include <fmt/ranges.h>
Expand Down Expand Up @@ -1188,6 +1189,12 @@ static PyObject* sc_provide(py_phlex_source* src, PyObject* args, PyObject* kwds
throw std::runtime_error("output specification error: " + msg);
}
}
auto ops = full_product_spec(
product_specification(algorithm_name::create(std::string_view(identifier(opq->creator))),
opq->suffix.value(),
opq->type),
identifier(opq->layer),
opq->stage.value_or(""_id));

// insert provider node (TODO: as in transform and observe, we'll leak the
// callable for now, until there's a proper shutdown procedure)
Expand All @@ -1197,47 +1204,47 @@ static PyObject* sc_provide(py_phlex_source* src, PyObject* args, PyObject* kwds
std::string const& out_type = output_types[0];
if (out_type == "bool") {
auto* pyc = new provider_cb_bool{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "int32_t") {
auto* pyc = new provider_cb_int{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "uint32_t") {
auto* pyc = new provider_cb_uint{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "int64_t") {
auto* pyc = new provider_cb_long{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "uint64_t") {
auto* pyc = new provider_cb_ulong{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "float") {
auto* pyc = new provider_cb_float{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type == "double") {
auto* pyc = new provider_cb_double{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (out_type.compare(0, 7, "ndarray") == 0 || out_type.compare(0, 4, "list") == 0) {
// TODO: just like for input types, these are hard-coded, but should be handled by
// an IDL instead.
std::string_view dtype{out_type.begin() + out_type.rfind('['), out_type.end()};
if (dtype == "[int32_t]") {
auto* pyc = new provider_cb_vint{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (dtype == "[uint32_t]") {
auto* pyc = new provider_cb_vuint{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (dtype == "[int64_t]") {
auto* pyc = new provider_cb_vlong{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (dtype == "[uint64_t]") {
auto* pyc = new provider_cb_vulong{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (dtype == "[float]") {
auto* pyc = new provider_cb_vfloat{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else if (dtype == "[double]") {
auto* pyc = new provider_cb_vdouble{callable};
src->ph_source->provide(functor_name, *pyc).output_product(opq.value());
src->ph_source->provide(functor_name, *pyc).output_product(ops);
} else {
PyErr_Format(PyExc_TypeError, "unsupported collection output type \"%s\"", out_type.c_str());
return nullptr;
Expand Down
Loading