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

Question: why next node is triggered without previous node call output.sendDone yet #1047

Open
linonetwo opened this issue Aug 17, 2023 · 3 comments

Comments

@linonetwo
Copy link
Contributor

I connect a TextField node to the ResultText node, and connect ResultText node to an Output node, and found even in TextField's process method I haven't call output.sendDone yet, ResultText's process method will keeps running when TextField receive IP (send by fbpGraph.addInitial to that TextField node). Why will ResultText be triggered by IP send to TextField node? And I found ResultText node is also triggering Output node by sending null to Output node's in port, why?

/* eslint-disable unicorn/no-null, @typescript-eslint/require-await */
// Load the NoFlo interface
import { Component } from 'noflo';
import type { IResultTextProps, UIEffectsContext } from '../types/UIEffectsContext';

export const getComponent = () => new ResultText();
class ResultText extends Component {
  description = 'Result text from workflow or LLM.';
  icon = 'check-square-o';
  uiElementID?: string;
  uiEffects?: UIEffectsContext;

  constructor() {
    super();

    this.inPorts.add('ui_effects', {
      datatype: 'object',
      description: 'Used by system, inject UI related methods.',
      required: true,
    });

    // Define the component's inports
    this.inPorts.add('in', {
      datatype: 'string',
      description: 'Result text to display.',
    });

    this.outPorts.add('out', {
      datatype: 'bang',
    });

    // Register a process handler for incoming data
    this.process(async (input, output) => {
      // DEBUG: console
      console.log(`process`);
      this.uiEffects ??= input.getData('ui_effects') as UIEffectsContext | undefined;
      if (this.uiEffects === undefined) return;
      if (!input.hasData('in')) return;
      const content = input.getData('in') as string;
      if (content === null || content === undefined) return;
      const props: IResultTextProps = {
        content,
      };
      this.uiElementID = this.uiEffects.addElement({ type: 'textResult', props });
      output.sendDone({ out: content });
    });
  }
}

GPT4:

Your described behavior and the code hint towards a few NoFlo nuances. Here's a breakdown:

  1. How does NoFlo handle data flow?
    The NoFlo runtime processes nodes in a data-driven manner. When you send an Information Packet (IP) to an input port of a component (e.g., TextField), NoFlo tries to activate the component by calling its process method. This behavior ensures that data flows through the graph.

  2. Why is ResultText activated when TextField receives an IP?
    If you've connected TextField's output port to ResultText's input port, sending an IP to TextField will activate it. When TextField sends an output (even if it doesn't call sendDone), that output activates the next node in line (ResultText in this case).

  3. Why is null sent to the Output node?
    If TextField sends a null (or any other value) to ResultText, and ResultText doesn't have the required data to process further (i.e., your checks like if (!input.hasData('in')) return;), it won't call output.sendDone(...). However, the IP might have already been forwarded to ResultText's output port. If you've connected ResultText to Output, then Output would receive that null.

Here are a few steps to help debug and possibly rectify the issue:

  1. Log IPs: Start by logging every IP that TextField sends out. This way, you can ensure that TextField is only sending what you expect.

  2. Handle IPs Carefully: In your ResultText process function, you're using input.getData('in'). This method not only fetches the IP but also removes it. If the conditions aren't right for processing that IP, it's essentially lost. Consider using input.get('in') to peek at the IP without removing it.

  3. IP Forwarding: Ensure you're only sending the IPs you intend to send. NoFlo's packet forwarding can sometimes lead to unintended data being sent if you're not careful. Ensure that any IP you send out is intended and controlled.

  4. Check Connections: Ensure your FBP graph connections are correctly made. An unintended connection might be causing the issues.

If you're still facing the issue after checking the above points, it might be helpful to look at the broader context, such as how your FBP graph is set up, how you're sending initial IPs, and if other components might be influencing this behavior.

@linonetwo
Copy link
Contributor Author

Human:
Why "sending an IP to TextField will activate ResultText"? I think "sending an IP to TextField will only activate TextField". And "When TextField sends an output (even if it doesn't call sendDone)", how can it send an putout without calling `sendDone`?


GPT4:
You're correct, I apologize for the oversight.

Let's clarify:

  1. Activation of Nodes: Sending an IP to TextField should indeed only activate TextField. The process function of TextField will be executed when it receives an IP. The subsequent nodes, like ResultText, should not activate unless they receive an IP themselves.

  2. Sending Data: In NoFlo, you use output.send() to send data from one component to the next. However, using output.sendDone() sends the data and also tells NoFlo that you've finished processing and the component can be deactivated for the current IP. This doesn't mean that data can't be sent using just output.send(). If you use output.send(), it will send the data downstream but won't indicate completion.

Given the confusion, let's address the core issue:

If ResultText is being activated when TextField receives an IP, a few things could be happening:

  1. Initial IPs: If you are sending initial IPs to multiple ports, it's possible that ResultText is being activated because it's receiving its own initial IP at roughly the same time.

  2. Multiple Send Calls: Ensure that TextField isn't making multiple calls to output.send() or output.sendDone() inadvertently.

  3. Graph Configuration: Ensure there's no other path in the graph that might be sending IPs to ResultText concurrently.

  4. Brackets: NoFlo uses IP brackets (< and >) to group related data. If you're using these, ensure that they are being opened and closed correctly. Sometimes, mismanaged brackets can cause odd behaviors in how data is processed.

If none of these seem to be the cause, there might be something deeper going on in the way your graph is set up or how the components are being used. You may want to set up a minimal reproducible example to narrow down the issue.


Human:
I think problem may be caused by IP Forwarding too. Please demonstrate how to do IP Forwarding in a noflo component, will it send `null` to next node when receiving IP from previous node (while not calling `output.sendDone()` and `output.send()`?


GPT4:
In NoFlo, IP forwarding typically means taking the incoming IP (Information Packet) and forwarding it to an output port, possibly without modifying it.

Here's a basic approach to IP forwarding:

this.process((input, output) => {
  // If we have data on 'in' port
  if (input.hasData('in')) {
    const data = input.getData('in');
    
    // Forward the IP to the 'out' port
    output.send({
      out: data
    });

    // Signal that processing for this IP is done
    output.done();
  }
});

However, if a component does not call output.send() or output.sendDone(), it shouldn't forward the IP or send any data to the next component in the flow. If it's doing so, there might be something else going on.

Regarding the null value: Forwarding IPs or the inner workings of NoFlo itself shouldn't arbitrarily send null to the next node. If you're receiving null values in the next component, it's most likely because null was sent as data from the previous node, or it's a result of how IPs are managed in your specific setup.

Remember, NoFlo's packet management can involve more than just data. There are different types of IPs, including:

  • Data IPs: Carries the actual data.
  • Open Bracket IPs: Denotes the start of a sequence of related IPs.
  • Close Bracket IPs: Denotes the end of a sequence.

If you're seeing unexpected behavior, make sure to account for all these different IPs and how they might be influencing the flow in your graph.


Human:

The answer is in noflo/src/lib/Component.js , if Component's process method (saved in this.handle) is an async function, that has res.then, it will send result of handle to output.sendDone. So even I'm not calling output.sendDone myself, I let the process method return undefined without blocking, so noflo automatically send an undefined (which turn to null somewhere) to next component.

  // ### Handling IP objects
  //
  // The component has received an Information Packet. Call the
  // processing function so that firing pattern preconditions can
  // be checked and component can do processing as needed.
  /**
   * @param {IP} ip
   * @param {InPort} port
   * @returns {void}
   */
  handleIP(ip, port) {
    if (!port.options.triggering) {
      // If port is non-triggering, we can skip the process function call
      return;
    }

    if ((ip.type === 'openBracket') && (this.autoOrdering === null) && !this.ordered) {
      // Switch component to ordered mode when receiving a stream unless
      // auto-ordering is disabled
      debugComponent(`${this.nodeId} port '${port.name}' entered auto-ordering mode`);
      this.autoOrdering = true;
    }

    // Initialize the result object for situations where output needs
    // to be queued to be kept in order
    /** @type {ProcessResult} */
    let result = {};

    if (this.isForwardingInport(port)) {
      // For bracket-forwarding inports we need to initialize a bracket context
      // so that brackets can be sent as part of the output, and closed after.
      if (ip.type === 'openBracket') {
        // For forwarding ports openBrackets don't fire
        return;
      }

      if (ip.type === 'closeBracket') {
        // For forwarding ports closeBrackets don't fire
        // However, we need to handle several different scenarios:
        // A. There are closeBrackets in queue before current packet
        // B. There are closeBrackets in queue after current packet
        // C. We've queued the results from all in-flight processes and
        //    new closeBracket arrives
        const buf = port.getBuffer(ip.scope, ip.index);
        const dataPackets = buf.filter((p) => p.type === 'data');
        if ((this.outputQ.length >= this.load) && (dataPackets.length === 0)) {
          if (buf[0] !== ip) { return; }
          if (!port.name) { return; }
          // Remove from buffer
          port.get(ip.scope, ip.index);
          const bracketCtx = this.getBracketContext('in', port.name, ip.scope, ip.index).pop();
          bracketCtx.closeIp = ip;
          debugBrackets(`${this.nodeId} closeBracket-C from '${bracketCtx.source}' to ${bracketCtx.ports}: '${ip.data}'`);
          result = {
            __resolved: true,
            __bracketClosingAfter: [bracketCtx],
          };
          this.outputQ.push(result);
          this.processOutputQueue();
        }
        // Check if buffer contains data IPs. If it does, we want to allow
        // firing
        if (!dataPackets.length) { return; }
      }
    }

    // Prepare the input/output pair
    const context = new ProcessContext(ip, this, port, result);
    const input = new ProcessInput(this.inPorts, context);
    const output = new ProcessOutput(this.outPorts, context);
    try {
      // Call the processing function
      if (!this.handle) {
        throw new Error('Processing function not defined');
      }
      const res = this.handle(input, output, context);
      if (res && res.then) {
        // Processing function returned a Promise
        res.then(
          (data) => output.sendDone(data),
          (err) => output.done(err),
        );
      }
    } catch (e) {
      this.deactivate(context);
      output.sendDone(e);
    }

    if (context.activated) { return; }
    // If receiving an IP object didn't cause the component to
    // activate, log that input conditions were not met
    if (port.isAddressable()) {
      debugComponent(`${this.nodeId} packet on '${port.name}[${ip.index}]' didn't match preconditions: ${ip.type}`);
      return;
    }
    debugComponent(`${this.nodeId} packet on '${port.name}' didn't match preconditions: ${ip.type}`);
  }

GPT4:
Thank you for diving into the NoFlo source code and sharing your findings!

The part you highlighted is particularly interesting:

const res = this.handle(input, output, context);
if (res && res.then) {
    // Processing function returned a Promise
    res.then(
        (data) => output.sendDone(data),
        (err) => output.done(err),
    );
}

From the code, it's clear that if your process function (this.handle) is asynchronous (returns a Promise), NoFlo will automatically take the resolution of that promise (in your case, undefined since your async function doesn't explicitly return anything) and send it using output.sendDone(data).

This behavior means that even if you don't explicitly send data, NoFlo will automatically send whatever your Promise resolves to (including undefined). This undefined value might get translated into null somewhere in the processing.

The implication of this is significant: if you don't want to send an output immediately after processing your async function, you'll need to handle it differently. Here are a few options:

  1. Explicitly Return: Return a specific value (other than undefined) if you want something meaningful to be sent to the next component.
  2. Send Output Manually: Instead of relying on the implicit behavior, you can use output.send(...) in your asynchronous code at the exact places where you want to send data.
  3. Avoid Using async Directly: Instead of using an async process function, you could use callbacks and manage your asynchronous operations within that. This might give you a more fine-grained control over when outputs are sent.

The key takeaway is that when working with NoFlo, especially with async functions, you need to be very intentional about when and what you return, since NoFlo might take action based on those returned values.


@linonetwo
Copy link
Contributor Author

This makes async process function and normal process function have different semantic about return value, maybe this should be changed in the core?

@linonetwo linonetwo reopened this Aug 17, 2023
@linonetwo
Copy link
Contributor Author

A workaround on user-side is to use legacy promise.then callback hell syntax, instead of new await.

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

No branches or pull requests

1 participant