Skip to content

fix #514, call_data_callbacks after shared the data #548

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

Jump to bottom
Draft
jmorice91 wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

fix #514, call_data_callbacks after shared the data #548

jmorice91 wants to merge 25 commits into from

Conversation

@jmorice91
Copy link
Contributor

@jmorice91 jmorice91 commented Mar 2, 2025 •
edited
Loading

!!!INSERT YOUR DESCRIPTION HERE!!!

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@jmorice91 jmorice91 linked an issue Mar 2, 2025 that may be closed by this pull request
Delay data events in multi_expose to when the last data is exposed #514
Open
@jmorice91 jmorice91 marked this pull request as draft March 2, 2025 21:48
@jmorice91 jmorice91 marked this pull request as ready for review March 13, 2025 21:43
@jmorice91 jmorice91 self-assigned this Mar 13, 2025
benoitmartin88
benoitmartin88 requested changes Mar 20, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

Great feature to add. Thanks !
Not much is needed to merge this PR.
I think this feature would require a unit test. Could you add this?

@jmorice91 jmorice91 marked this pull request as draft April 4, 2025 08:43
@jmorice91 jmorice91 force-pushed the 514-delay-data-events-in-multi_expose branch from 656e28b to 3e1d45f Compare April 4, 2025 09:23
@jmorice91 jmorice91 requested a review from JAuriac April 9, 2025 13:27
@jmorice91 jmorice91 marked this pull request as ready for review April 9, 2025 13:27
benoitmartin88
benoitmartin88 requested changes Apr 14, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

Almost ready to merge.
I do have a few questions that need answering concerning error handling, potentially superfluous functions and writing tests in C++.

@jmorice91 jmorice91 marked this pull request as draft May 12, 2025 09:07
@jmorice91 jmorice91 force-pushed the 514-delay-data-events-in-multi_expose branch from f542e39 to 6806742 Compare May 12, 2025 09:18
@jmorice91 jmorice91 requested a review from benoitmartin88 May 12, 2025 09:45
@jmorice91 jmorice91 marked this pull request as ready for review May 12, 2025 09:46
benoitmartin88
benoitmartin88 requested changes Jun 4, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

I think this is almost ready to merge.
Could you make sure the indent and spacing of the modified files is correct ?
Thanks !

Yushan-Wang
Yushan-Wang previously approved these changes Jun 4, 2025
View reviewed changes
@jmorice91
Copy link
Contributor Author

jmorice91 commented Jun 4, 2025

I think this is almost ready to merge. Could you make sure the indent and spacing of the modified files is correct ? Thanks !

Is it normal that the action indent doesn't catch that?

@jmorice91 jmorice91 requested a review from benoitmartin88 June 5, 2025 10:12
Yushan-Wang
Yushan-Wang approved these changes Jun 19, 2025
View reviewed changes
Copy link
Member

@Yushan-Wang Yushan-Wang left a comment

Choose a reason for hiding this comment

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

Ready to merge

@Yushan-Wang
Copy link
Member

Yushan-Wang commented Jul 16, 2025

@jbigot Could you review this PR please?

jbigot
jbigot requested changes Sep 3, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

This is a great submission! My main comment is about moving Var_to_reclaim of the provider side instead of expecting each user to implement it. Overall this is really nice though!

Yushan-Wang
Yushan-Wang previously approved these changes Sep 12, 2025
View reviewed changes
@jbigot
Copy link
Member

jbigot commented Oct 28, 2025

What's the status for this one?

@jmorice91
Copy link
Contributor Author

jmorice91 commented Oct 31, 2025

What's the status for this one?

It is ready to review.

@jbigot
Copy link
Member

jbigot commented Nov 3, 2025

@Yushan-Wang , @benoitmartin88 , @JAuriac : I'd like a cross review first and then I'll do one. Who has time for one?

@benoitmartin88
Copy link
Member

benoitmartin88 commented Nov 3, 2025

Je regarde.

Yushan-Wang
Yushan-Wang reviewed Nov 5, 2025
View reviewed changes
pdi/include/pdi/context.h

#include <pdi/pdi_fwd.h>
#include <pdi/callbacks.h>
//#include <pdi/delayed_data_callbacks.h>
Copy link
Member

@Yushan-Wang Yushan-Wang Nov 5, 2025

Choose a reason for hiding this comment

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

remove commented-out lines

Copy link
Contributor Author

@jmorice91 jmorice91 Nov 10, 2025

Choose a reason for hiding this comment

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

This will be done in the next commit

benoitmartin88
benoitmartin88 requested changes Nov 14, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. There are a few changes that are required.

pdi/include/pdi/context.h

#include <pdi/pdi_fwd.h>
#include <pdi/callbacks.h>
//#include <pdi/delayed_data_callbacks.h>
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 3, 2025

Choose a reason for hiding this comment

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

Why is this commented ?

Copy link
Contributor Author

@jmorice91 jmorice91 Nov 17, 2025

Choose a reason for hiding this comment

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

This will be remove in the next commit

pdi/include/pdi/delayed_data_callbacks.h
class PDI_EXPORT Delayed_data_callbacks
{
/// friend class Global_context;
std::list<std::string> m_datanames;
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 4, 2025

Choose a reason for hiding this comment

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

You are iterating over all the elements of this data structure. It probably is better if this is a std::forward_list.

Copy link
Contributor Author

@jmorice91 jmorice91 Nov 18, 2025

Choose a reason for hiding this comment

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

For the logger message, I need the size of the list. In std::forward_list, we don't have access to the number of elements.

Copy link
Member

@Yushan-Wang Yushan-Wang Nov 19, 2025

Choose a reason for hiding this comment

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

If the size is only used for logger, we can do it differently e.g. nb_elemt = std::distance(m_datanames.begin(), m_datanames.end())

pdi/src/data_descriptor_impl.h

struct PDI_NO_EXPORT Ref_holder;

public:
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

why public ?

pdi/src/global_context.h
#include "pdi/callbacks.h"
#include "pdi/context.h"
#include "pdi/context_proxy.h"
//#include "pdi/delayed_data_callbacks.h"
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

remove

pdi/src/pdi.cxx
/** A structure to reclaim the datas properly in case of error
*/
struct Var_to_reclaim {
std::list<string> m_varnames;
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

I think that a vector here makes more sense.

pdi/src/pdi.cxx
std::list<string> m_varnames;
Var_to_reclaim() = default;

~Var_to_reclaim()
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

Suggested change
~Var_to_reclaim()
~Var_to_reclaim() noexcept

pdi/tests/PDI_data_descriptor.cxx
PC_tree_t array_config{PC_parse_string("{ size: 10, type: array, subtype: int }")};
shared_ptr<Array_datatype> array_datatype{Array_datatype::make(Scalar_datatype::make(Scalar_kind::SIGNED, sizeof(int)), 10)};
PDI::Paraconf_wrapper fw;
//Global_context global_ctx{PC_parse_string("logging: trace")};
Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

remove

Yushan-Wang
Yushan-Wang reviewed Nov 26, 2025
View reviewed changes
pdi/include/pdi/delayed_data_callbacks.h
class PDI_EXPORT Delayed_data_callbacks
{
/// friend class Global_context;
std::list<std::string> m_datanames;
Copy link
Member

@Yushan-Wang Yushan-Wang Nov 19, 2025

Choose a reason for hiding this comment

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

If the size is only used for logger, we can do it differently e.g. nb_elemt = std::distance(m_datanames.begin(), m_datanames.end())

pdi/tests/PDI_callbacks.cxx
Ref_w ref_write{ref};
int* x = static_cast<int*>(ref_write.get());
*x += 42;
std::cout << "name=" << name.c_str() << " == data_x" << std::endl;
Copy link
Member

@Yushan-Wang Yushan-Wang Nov 19, 2025

Choose a reason for hiding this comment

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

remove stdcout and iostream

@jmorice91 jmorice91 marked this pull request as draft December 3, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yushan-Wang Yushan-Wang Yushan-Wang left review comments

@benoitmartin88 benoitmartin88 benoitmartin88 requested changes

@JAuriac JAuriac Awaiting requested review from JAuriac

@jbigot jbigot Awaiting requested review from jbigot jbigot is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@jmorice91 jmorice91

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Delay data events in multi_expose to when the last data is exposed

5 participants

@jmorice91 @Yushan-Wang @jbigot @benoitmartin88