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

dyn-6672 - python engines failing to load should not stop Dynamo from starting #3040

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Feb 14, 2024

Purpose

DynamoDS/Dynamo#14891

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • Snapshot of UI changes, if any.

@@ -147,7 +147,6 @@
</Reference>
</ItemGroup>
<ItemGroup>
<Content Include="Ionic.Zip.xml" />
Copy link
Member Author

Choose a reason for hiding this comment

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

this file was deleted previously.

catch(FileNotFoundException ex)
{
Logger.Log(ex);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really wish there was a DynamoCore way to ensure that all PythonEngine public APIs are safe to use
If we go with this route, I think we might have to do the same thing for other host integrators

Copy link
Collaborator

@pinzart90 pinzart90 Feb 14, 2024

Choose a reason for hiding this comment

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

You cannot even mark an event as "Can throw Exception of type X". Oh we can...if that is any comfort

Copy link
Member Author

Choose a reason for hiding this comment

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

@pinzart90 I am looking into something on the DynamoCore side, I will send another PR for feedback on that. More a spike than something real at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to propagate the error in DynamoCore and display it to the user somehow?

@Mikhinja Mikhinja merged commit cde18b5 into DynamoDS:master Feb 20, 2024
9 checks passed
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

Successfully merging this pull request may close these issues.

3 participants