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

Dynamo ML data ingestion Pipeline Extension. #14749

Merged
merged 22 commits into from
Jan 30, 2024

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Dec 12, 2023

Purpose

Task: https://jira.autodesk.com/browse/DYN-6294.

We will be using Dynamo ML data ingestion pipeline as an extension inside Dynamo. It will be using the same restsharp 108.0.1 version only. This will be controlled by a feature flag and will be turned off for now.

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.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

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

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

@zeusongit
Copy link
Contributor

do we handle the NoNetwork scenario in this PR?

</dependentAssembly>
</assemblyBinding>
</runtime>
</configuration>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file needed?

@pinzart90
Copy link
Contributor

We need to make sure the NoNetworkMode works on this new view extension

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

Copy link

UI Smoke Tests

Test: failure. 0 passed, 2 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

Copy link

github-actions bot commented Dec 13, 2023

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang
Copy link
Contributor

Build error on action
D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\ViewModels\Core\DynamoViewModel.cs(792,17): error CS0103: The name 'Analytics' does not exist in the current context [D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj]

@reddyashish
Copy link
Contributor Author

Build error on action D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\ViewModels\Core\DynamoViewModel.cs(792,17): error CS0103: The name 'Analytics' does not exist in the current context [D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj]

Fixed it now

@reddyashish
Copy link
Contributor Author

Triggered the job again. Also running the job on master15: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1399/

@QilongTang
Copy link
Contributor

Merge for testing and future performance improvements

@QilongTang QilongTang merged commit 2c851b1 into DynamoDS:master Jan 30, 2024
22 checks passed
QilongTang pushed a commit that referenced this pull request Jan 30, 2024
* commit

* updates

* updates

* Update DynamoCoreWpf.csproj

* Update DynamoCore.csproj

* remove extra whitespaces

* updates

* updates

* new changes

* switch to prod

* add to dynamocore.sln

* Control by feature flag.

* Update DynamoMLDataPipelineExtension.cs

* Update DynamoMLDataPipelineExtension.cs

* Update DynamoMLDataPipeline.csproj

* Separating the extension code from core code and addressing comments.

* Update DynamoMLDataPipeline.csproj

* Update to feature flag and some comments

* Fix Analytics reference error

* Fix failing tests.
QilongTang added a commit that referenced this pull request Jan 30, 2024
…14911)

* Dynamo ML data ingestion Pipeline Extension. (#14749)

* commit

* updates

* updates

* Update DynamoCoreWpf.csproj

* Update DynamoCore.csproj

* remove extra whitespaces

* updates

* updates

* new changes

* switch to prod

* add to dynamocore.sln

* Control by feature flag.

* Update DynamoMLDataPipelineExtension.cs

* Update DynamoMLDataPipelineExtension.cs

* Update DynamoMLDataPipeline.csproj

* Separating the extension code from core code and addressing comments.

* Update DynamoMLDataPipeline.csproj

* Update to feature flag and some comments

* Fix Analytics reference error

* Fix failing tests.

* Remove beta keyword

* Make the test as failure

---------

Co-authored-by: reddyashish <[email protected]>
<?xml version="1.0" encoding="utf-8"?>
<configuration>
<appSettings>
<add key="StagingClientUrl" value="https://developer-stg.api.autodesk.com/exchange"/>
Copy link
Member

Choose a reason for hiding this comment

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

@reddyashish @QilongTang this file should not be here.

</ProjectReference>
</ItemGroup>
<ItemGroup>
<Reference Include="System" />
Copy link
Member

Choose a reason for hiding this comment

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

these should not be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

<RootNamespace>DynamoMLDataPipeline</RootNamespace>
<AssemblyName>DynamoMLDataPipeline</AssemblyName>
<FileAlignment>512</FileAlignment>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
Copy link
Member

Choose a reason for hiding this comment

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

should not be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. #15116

<Reference Include="System.Xml" />
</ItemGroup>
<ItemGroup>
<Compile Include="DynamoMLDataPipeline.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

totally unnecessary in sdk style, remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. #15116

<Compile Include="Properties\AssemblyInfo.cs" />
</ItemGroup>
<ItemGroup>
<None Include="App.config" />
Copy link
Member

Choose a reason for hiding this comment

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

remove this.

namespace DynamoMLDataPipeline
{

internal class DynamoMLDataPipelineExtension : IExtension, IExtensionSource
Copy link
Member

Choose a reason for hiding this comment

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

why is this an IExtensionSource which throws not implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove this after some testing. Removed here: #15116


public void Shutdown()
{
throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

these should be empty, they should not throw...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated #15116

namespace DynamoMLDataPipeline
{
// Schema and the assets for the data request body.
class UploadAssetsRequestBody
Copy link
Member

Choose a reason for hiding this comment

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

You really should use access modifiers to make this explicit, I think default is private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default is internal for classes. Added internal modifier to these classes explicitly. #15116.

public void Startup(StartupParams sp)
{
DynamoMLDataPipeline = new DynamoMLDataPipeline();
DynamoMLDataPipeline.AuthTokenProvider = (IOAuth2AccessTokenProvider)sp.AuthProvider;
Copy link
Member

Choose a reason for hiding this comment

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

is this the only reason this extension exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

as opposed to being in an external process ? like the md2html.exe...

@mjkkirschner
Copy link
Member

on @zeusongit's question, it seems no network mode was not handled?

@reddyashish
Copy link
Contributor Author

on @zeusongit's question, it seems no network mode was not handled?

Will address it in the followup PR. Won't be affected by the current changes.

Also will address the remaining comments in the followup PR for master branch.

@pinzart90
Copy link
Contributor

Is the purpose of this extension to strip sensitive data out of the dyn and send it to some cloud storage ?
Is there back and forth communication expected ?...
I am probably missing something...

@reddyashish
Copy link
Contributor Author

reddyashish commented Jan 30, 2024

This extension is just sending the necessary dyn info to the Forge cloud storage, using restsharp API. No info is expected in the response, just an acknowledgement when it has successfully received.
That info will used to train the NodeAutocomplete ML model

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.

5 participants