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

Add support for debug modes #10603

Merged
merged 10 commits into from
May 4, 2020
Merged

Add support for debug modes #10603

merged 10 commits into from
May 4, 2020

Conversation

pinzart
Copy link
Contributor

@pinzart pinzart commented Apr 26, 2020

Purpose

Add support for debug modes (similar to feature flags).
The main idea behind this PR is to allow any developer to wrap functionality under flags. The code can get pushed to master...and not interfere with other people's work. It can even get shipped to users. As long as the "experimental code" is wrapped under a DebugMode and not activated...it should not change any of the existing functionality.

Declarations

Check these if you believe they are true

  • The codebase 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
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@pinzart pinzart added the DNM Do not merge. For PRs. label Apr 26, 2020
<configSections>
<section name="DebugModes" type="System.Configuration.AppSettingsSection"/>
</configSections>

Copy link
Contributor Author

@pinzart pinzart Apr 26, 2020

Choose a reason for hiding this comment

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

To enable a debug mode (from config file)...one would have to add a new entry under a DebugModes section like so:
<DebugModes> <add key="DebugMode1" value="true"/> <add key="DebugMode2" value="true"/> </DebugModes>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a separate config file...(debug.config) just so that we do not have to pollute the DynamoCore config ...

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 26, 2020

Hi @pinzart a few questions about the context of this PR:
What work does this this pull request support?
What modes do you wish to enable or disable via config file?
What about the existing debug menu does not satisfy the needs of your work?

@QilongTang
Copy link
Contributor

@pinzart For local testing seems the current debug mode is enough unless you plan to distribute Dynamo in such a way?

@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

What work does this this pull request support?
@mjkkirschner The main idea behind this PR is to allow developers to wrap functionality under flags (basically feature flags). THe code can get pushed to master...and not interfere with other people's work. It can even get shipped to users. As long as the "experimental code" is wrapped under a DebugMode and not activated...it should not change any of the existing functionality.

What modes do you wish to enable or disable via config file?
I am hoping to add Debug modes for ADP (ex: EnableADPAnalytics, EnableADPOptinPage etc)

What about the existing debug menu does not satisfy the needs of your work?
THe debug menu is good for a new UI (a list of available debugmodes ...displayed as checkboxes + name + description). Exactly like Revit has...

@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

@QilongTang
For local testing seems the current debug mode is enough unless you plan to distribute Dynamo in such a way?
This PR does not add any debug mode. It is just the infrastructure to support adding debug modes. By local testing.,..you mean testing the infrastructure code of the current PR...or the functionality behind new debug modes that a developer could add down the line ?

reported err:
Configuration\DebugModes.cs(55,94): error CS1525: Invalid expression term 'bool'
Dynamo builder probably has older compilers (that does not support inlined var declaration)
@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

This is what the UI (DebugModes window) would look like
image

@pinzart pinzart removed the DNM Do not merge. For PRs. label Apr 27, 2020
@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

@mjkkirschner @QilongTang any more questions/thoughts on this ?

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 27, 2020

@pinzart I still don't understand the relationship between this feature and the debug menu we already have.

Are you going to unify them? I much prefer not to have two competing debug feature flag menu options...

@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

Here is how I merged the DebugModes with the DebugMenu
image

DebugTab -> DebugMenu -> DebugModesMenuItem => DebugModesWindow

@pinzart
Copy link
Contributor Author

pinzart commented Apr 27, 2020

@mjkkirschner let's talk tomorrow so I can better explain my changes

@pinzart
Copy link
Contributor Author

pinzart commented Apr 29, 2020

Added unit tests and switched to basic xml configuration file (makes it easier to test and handle).
debug.config should not be delivered as part of any release . It only makes sense to manually add it when you want to enable debug modes on startup.

@pinzart
Copy link
Contributor Author

pinzart commented May 4, 2020

@mjkkirschner @QilongTang any more issues with this PR ?

@QilongTang
Copy link
Contributor

LGTM

@QilongTang QilongTang added the LGTM Looks good to me label May 4, 2020
@pinzart pinzart merged commit 36496ff into master May 4, 2020
@pinzart pinzart deleted the debug_modes branch May 4, 2020 18:58
reddyashish added a commit that referenced this pull request May 14, 2020
* add namespace conflict tests for short name replacer and node to code (#10611)

* Register custom node before package load reset (#10591)

This fixes a problem where existing custom nodes in the home workspace
became unresolved after a package that contained binaries was loaded.

The cause of the problem was that the compiled function was not
available at the time of the execution after a VM reset. Now the data
is registered on package load, by queueing it in
pendingCustomNodeSyncData. This results in a CompileCustomNodeAsyncTask
being scheduled before the update of the home workspace graph takes
place.

* Increase coverage of the Core folder (#10609)

* Add a test for the CrashPromptArgs class

* Update DynamoCoreTests.csproj

* Added NotificationObject tests

* Updated the test, NotificationObject coverage at 100%

* Changed some access modifiers

* Added coverage for Updates/BinaryVersion

* Added comments and fixed names

* Revert "Added comments and fixed names"

This reverts commit 42cd024.

* Revert "Added coverage for Updates/BinaryVersion"

This reverts commit 679deca.

* Increased coverage in CrashPromptArgs

* Added some Core coverage

* DYN-2560 - Increase the code coverage: DynamoCore/Models Folder First Part (#10612)

* DYN-2560 - Increase the code coverage: DynamoCore/Models Folder

I started adding just one test method TestOnRequestDispatcherBeginInvoke() for testing the DynamoModelEvents class.

* DYN-2560 - Adding test cases for DynamoModelEvents

Adding test cases for DynamoModelEvents

* DYN-2560 - Adding test cases for DynamoModelEvents

I added all the test cases for the all events  in the DynamoModelEvent class, i just need to fix the last 6 of them.

* DYN-2560 - Adding test cases for DynamoModelEventsArgs

I added several test cases for the classes inside the DynamoModelEventsArgs.cs file.

     ZoomEventArgs
     TaskDialogEventArgs
     EvaluationCompletedEventArgs
     DynamoModelUpdateArgs
     FunctionNamePromptEventArgs
     PresetsNamePromptEventArgs
     ViewOperationEventArgs
     PointEventArgs
     WorkspaceEventArgs
     ModelEventArgs

* DYN-2560 - Code Review Comments

Based in the comment done by Aaron in the GitHub pull request, I added more description comments for the method TestTaskDialogEventArgs() and also I added comments for a local function

* DYN-2560 - Code Review Comments 2

There was a spelling error in two methods for the word "Internally", then I fixed this error in the two places.

* Python Engine Enum (#10618)

* Cherrypick

* Comments

* Add unit test

* Comments

* Handle runtime table gaps on code block deletion (#10605)

When the runtime table are built there is an implicit assumption that
the code block ids are consecutive. However, that is not always the
case, as the deletion of a procedure causes the deletion of its child
code blocks, which may generate gaps in the id numbering.

In order to make the code resiliant to these gaps, the runtime tables
are sized based on the largest code block id, rather than in the amount
of code blocks.

* Validate ASM installations before loading (#10621)

The check for the specific assemblies tbb.dll and tbbmalloc.dll is
generalized to a full file list validation of detected ASM locations.
This way, Dynamo is guarded against any incomplete/unusual ASM binary
folders that other applications might include.

The lists of files for each version were taken from LibG. They cannot
be reused from LibG without involving major changes in the preloader,
so the lists should be kept in sync as new major release of ASM occur.

* SQ bug fix (#10622)

* (1) Null reference bug fix from SQ dashboard

* Add support for debug modes (#10603)

* Add support for debug modes

* Increase coverage of the Updates folder (#10628)

* Add a test for the CrashPromptArgs class

* Update DynamoCoreTests.csproj

* Added NotificationObject tests

* Updated the test, NotificationObject coverage at 100%

* Changed some access modifiers

* Added coverage for Updates/BinaryVersion

* Added comments and fixed names

* Added asserts for happy path, changed namespace and deleted a console function

* Removed using

* Revert "Removed using"

This reverts commit 022823d.

* Removed a change that should not be there

* Added coverage for Updates folder

* Tests for CPython3 Engine as well as for IronPython.

* [WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. (#10624)

* reduce number of serialization tests by factor 3~

* reduce wpf json serialization tests

* Handle missing instance calling method statically (#10630)

A code block node calling an instance method in its static form would
make Dynamo crash if the instance was not provided. An example of this
would be calling 'Curve.Patch();'.

The cause of the issue was that the default argument was ultimately
tried to be interpreted as a pointer. By avoiding that wrong conversion
the engine is now able to surface the real problem as a human-readable
warning.

* Python3 Selection Under Debug Modes (#10629)

* Cherrypick Python3 changes

* Cleanup

* Use Debug Modes

* CleanUp

* Rename Function

* Clean Up

* Do not use anouymous function as handler

* Revert newer language change

* Add adp analytics to Dynamo (#10576)

* add ADPTracker register

* Fix non-array item search for dot operation (#10633)

When an array is passed to the dot operation, it needs to get an item
to determine the actual class being processed. This was done in
ArrayUtils.GetFirstNonArrayStackValue, but the function only checked
the first item of the array and its descendants. This caused the dot
operation to failed when passed an array that contained an empty array
as its first item.

In order to fix the problem, the function was changed to check for
non-array items in the entire array. The function should stop as soon
as the first non-array item is found.

* Addressing some comments

* some more comments

* Update AssemblyInfo.cs

* Update AssemblyInfo.cs

* Marking the python node as modified when its engine property is modified

Also updated some tests.

* Remove unwanted check.

* changes to test

* Update AssemblySharedInfo.cs

* Using python engine's AcquireLock to avoid deadlock.

The deadlock was happening only when multiple PythonEngine's are initialized on a thread from 2 different test fixtures in the same run.
The main difference is calling this function PythonEngine.BeginAllowThreads().
Also we do not want to initialze the python engine if it is already initialized.

Co-authored-by: aparajit-pratap <[email protected]>
Co-authored-by: Martin Misol Monzo <[email protected]>
Co-authored-by: Bruno Yorda <[email protected]>
Co-authored-by: Roberto T <[email protected]>
Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Ashish Aggarwal <[email protected]>
Co-authored-by: Tibi <[email protected]>
Co-authored-by: Michael Kirschner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants