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

fix: add ignore_error executor field #176

Open
athith-g opened this issue Jun 7, 2024 · 2 comments
Open

fix: add ignore_error executor field #176

athith-g opened this issue Jun 7, 2024 · 2 comments

Comments

@athith-g
Copy link
Contributor

athith-g commented Jun 7, 2024

Describe the bug
TES v1.1 defines an ignore_error field as a property of an executor (defined here). Currently, the TesExecutor model does not have this attribute. As a result, the following error occurs when a response with the ignore_error is provided:

TypeError: Executor.__init__() got an unexpected keyword argument 'ignore_error'

Full traceback:
Screenshot 2024-06-07 at 3 34 46 PM

Executor causing error:
Screenshot 2024-06-07 at 3 35 34 PM

Software
Using proTES with Funnel

@uniqueg
Copy link
Member

uniqueg commented Jun 9, 2024

Thanks for reporting. This issue is due to Funnel, but not proTES, having adopted TES v1.1.0. The library that throws this error is py-tes.

A few points:

  1. Upgrading py-tes to use the v1.1.0 branch should resolve this issue, but it's possible that we might run into other issues. For example, proTES may not forward the ignore_error value.
  2. Alternatively, downgrading Funnel to a version that adopts v1.0.0 should also fix the issue. However, as far as I know, the latest Funnel version addresses a number of issues and is not just an upgrade to TES v1.1.0, so this would rather be a short-term solution.
  3. The best way forward is probably to upgrade the proTES models to properly support v1.1.0, but making sure that it remains backwards-compatible.
  4. Apart from dealing with ignore_error in one way or another, py-tes should not actually throw this error. This is because additionalProperties is not set to false in the TES specs, meaning that returning additional specs is not violating the specs and should not throw an error. We should open an issue about that.

Do you want to take care of these issues, @athith-g? That is:

  • Pick one of solutions 1.-3. to make the immediate problem go away.
  • Open an issue in the py-tes repo to let Liam and Kyle know that they should handle additional properties gracefully.

@athith-g
Copy link
Contributor Author

@uniqueg For sure I'll update the proTES models and open up that issue.

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

2 participants