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

feat(core): Add batching and other options to declarative nodes #8885

Merged
merged 15 commits into from
Jun 7, 2024

Conversation

janober
Copy link
Member

@janober janober commented Mar 13, 2024

Summary

Adds support for

  • Batch processing
  • Ignore SSL Issues
  • Proxy
  • Timeout

to all declarative nodes.
Screenshot from 2024-03-24 11-57-54

FYI:

  1. Instead of getting added under "Options" to the regular Parameters does it get added under Node-Settings. The reason is the parameters get displayed and hidden on nodes depending on the displayOptions. So do different Resource -> Operation combinations often share parameters or have totally separate ones.
    As there is no easy way (for sure possible, but none that would not increase the scope of this originally simple and fast extension) of ensuring that the options get reliably displayed properly for all combinations, did we decide to move it into Node-Settings.
  2. OpenAI Node: As the OpenAI-Node got reimplemented a while ago and does not now use the declarative style anymore, will it only be available if the old node does get used.

Related tickets and issues

https://community.n8n.io/t/langchain-and-openai-nodes-should-be-able-to-parallel-requests-like-http-node/40383/2

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Mar 13, 2024
@pemontto
Copy link
Contributor

Excited to see this! A few questions if you don't mind:

  1. Will the batching settings be configurable by the end-user, node developer, or both?
  2. Will there be a mechanism to handle back-off when we receive 429s?

@janober janober marked this pull request as ready for review March 16, 2024 18:57
@janober
Copy link
Member Author

janober commented Mar 16, 2024

@pemontto to your questions

  1. What this PR does is to expose batch options by default via the UI to the users. Currently, there is no way for node developer to overwrite it or set different defaults.
  2. No sadly not

Both things could obviously be added in the future if there is a strong demand

@janober janober marked this pull request as draft March 16, 2024 20:57
@janober janober marked this pull request as ready for review March 18, 2024 10:42
Copy link
Contributor

@michael-radency michael-radency left a comment

Choose a reason for hiding this comment

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

LGTM, I had to add // eslint-disable-next-line complexity for runNode, but I guess this is expected here

Copy link

cypress bot commented Jun 6, 2024

1 flaky test on run #5332 ↗︎

0 358 0 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 janober 🗃️ e2e/*
Project: n8n Commit: 15523f173f
Status: Passed Duration: 04:46 💡
Started: Jun 6, 2024 7:31 AM Ended: Jun 6, 2024 7:36 AM
Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video

Review all test suite changes for PR #8885 ↗︎

Copy link
Contributor

github-actions bot commented Jun 6, 2024

✅ All Cypress E2E specs passed

@michael-radency michael-radency merged commit 4e56863 into master Jun 7, 2024
28 checks passed
@michael-radency michael-radency deleted the add-batching-to-declarative-style branch June 7, 2024 05:39
MiloradFilipovic added a commit that referenced this pull request Jun 7, 2024
* master:
  feat(core): Add batching and other options to declarative nodes (#8885)
  fix: Reset pagination when output size changes (#9652)
RicardoE105 pushed a commit that referenced this pull request Jun 9, 2024
Co-authored-by: Michael Kret <michael.k@radency.com>
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
@janober
Copy link
Member Author

janober commented Jun 12, 2024

Got released with n8n@1.46.0

@juniorgregio
Copy link

@janober good evening, is this adjustment now available for the community edition?
I just updated to 1.46.0, but the OpenAI node does not find this batching option at any time

@janober
Copy link
Member Author

janober commented Jun 24, 2024

It is available in all versions. The problem is just hat the OpenAI node got changed for different reasons and is now not declarative anymore for that reason did it not work with that node anymore. Will however work with v1 of the node. So if you change it to that one it should work. Let me know if you do not know how.

@juniorgregio
Copy link

It is available in all versions. The problem is just hat the OpenAI node got changed for different reasons and is now not declarative anymore for that reason did it not work with that node anymore. Will however work with v1 of the node. So if you change it to that one it should work. Let me know if you do not know how.

Thank you for the quick return, I don't know how to use the node v1. Could you teach me? Thanks

@juniorgregio
Copy link

@janober is there any desire or plan to make openai declative again?

about changing the node version, I used GPT and it told me that I need to change the configuration files on the server, is that it?

@janober
Copy link
Member Author

janober commented Jul 1, 2024

@juniorgregio Sorry, did miss your answer.

To get version 1 of the node you can simply copy the following and paste it into n8n:

{
  "nodes": [
    {
      "parameters": {
      },
      "id": "ef1a6b15-1248-446e-a61d-fc93192420c0",
      "name": "OpenAI",
      "type": "n8n-nodes-base.openAi",
      "typeVersion": 1,
      "position": [
        420,
        460
      ]
    }
  ],
  "connections": {},
  "pinData": {}
}

Sadly, it is not currently planned to make it declarative again, as it would bring other disadvantages that are very specific to the node and its usage (details would be too complex to explain right now). Sorry.

@juniorgregio
Copy link

juniorgregio commented Jul 1, 2024

Desculpe, faltou sua resposta.

Para obter a versão 1 do nó, você pode simplesmente copiar o seguinte e colá-lo em n8n:

{
  "nodes": [
    {
      "parameters": {
      },
      "id": "ef1a6b15-1248-446e-a61d-fc93192420c0",
      "name": "OpenAI",
      "type": "n8n-nodes-base.openAi",
      "typeVersion": 1,
      "position": [
        420,
        460
      ]
    }
  ],
  "connections": {},
  "pinData": {}
}

Infelizmente, atualmente não está planejado torná-lo declarativo novamente, pois traria outras desvantagens que são muito específicas para o nó e seu uso (detalhes seriam muito complexos para explicar agora). Desculpa.

Thanks for the feedback @janober , it's helped a lot but I still have questions about where exactly to paste it on the N8N?

@pemontto
Copy link
Contributor

pemontto commented Jul 1, 2024

@juniorgregio straight onto the workflow canvas! 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants