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

Workflow cleanup and various fixes #47

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

PathogenDavid
Copy link
Member

@PathogenDavid PathogenDavid commented Jan 8, 2025

Merge bonsai-rx/machinelearning-examples#14 before this one.


This PR:

  • Unifies the build and test GitHub Actions workflows
  • Cleans up the GitHub Actions workflows (including fixing PSA: Old versions of actions/setup-dotnet will break soon bonsai#2091)
  • Fixes test failures caused by Python 3.1 being used instead of Python 3.10
  • Adds building and testing of Linux (via Mono) and the debug configuration
  • Removes .NET Standard
    • Projects should either multi-target Framework and modern .NET or target only .NET Standard, not both.
  • Targets tests to .NET Framework instead of .NET 8
  • Migrates the documentation deployment to use the modern deployment strategy
  • Cleaned up and fixed various issues with ReceptiveFieldSimpleCellTest
  • Fixes the formatting of the Visual Studio Solution
  • Updates the examples repo to fix the documentation workflow (needs Fix malformed Bonsai.configs machinelearning-examples#14)

Supersedes #44

@ncguilbeault
Copy link
Collaborator

Everything looks good to me.

@glopesdev glopesdev self-requested a review March 7, 2025 12:49
@PathogenDavid PathogenDavid force-pushed the workflow-cleanup-and-fixes branch from 8d10963 to 0626a53 Compare March 7, 2025 14:32
@PathogenDavid
Copy link
Member Author

Rebased on current main :shipit:

@@ -3,7 +3,7 @@
<Title>Bonsai.ML.Data</Title>
<Description>Provides common tools and functionality for working with ML data.</Description>
<PackageTags>Bonsai Rx ML Machine Learning Data</PackageTags>
<TargetFrameworks>net472;netstandard2.0</TargetFrameworks>
<TargetFramework>net472</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we are removing netstandard2.0? I think I remember we discussed this at some point, but I don't see any issue with supporting it, this is basically a pathway to allow consuming these libraries from command-line tools, similar to Harp and other core packages.

Copy link
Member

@glopesdev glopesdev Mar 12, 2025

Choose a reason for hiding this comment

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

@PathogenDavid Reading your PR summary, I now understand better the intention, and wanted to give a bit of context to the original design.

The reason net472 is even there to begin with is Rx backwards compatibility. Long-story short we decided to keep targeting Rx-Linq 2.2.5 for all .NET framework targets and System.Reactive for all modern .NET targets. Sadly these two packages have been made fundamentally incompatible over the years as they share identically named types with different assembly public key tokens.

The reason we chose netstandard2.0 instead of net8.0 is because this is the lowest API compatibility we require (since we don't need anything specific to .NET core anyway). Applications targeting .NET framework will always prefer net472 while applications targeting net8.0 will always go for the netstandard2.0 target.

We need this compatibility with modern .NET since we sometimes leverage using these libraries in CLI tools made to be used with dotnet, and in general we want to keep moving our dependency stack away from the legacy .NET framework.

I'm happy to have a broader discussion about this, but maybe these changes should live for now in a separate issue / PR so we can discuss the pros and cons separately from the CI workflow cleanup?

Copy link
Member Author

@PathogenDavid PathogenDavid Mar 12, 2025

Choose a reason for hiding this comment

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

I don't remember the specifics anymore, but IIRC the .NET Standard build of Bonsai.ML was basically nonfunctional and was causing problems after fixing gaps not covered by the old CI. It was also part of what was breaking the TorchSharp branch on Linux.

We should definitely have a wider discussion about this at some point because .NET Standard is meant as an alternative to multi-targeting. It rarely makes actual sense to use them together.

We need this compatibility with modern .NET since we sometimes leverage using these libraries in CLI tools made to be used with dotnet, and in general we want to keep moving our dependency stack away from the legacy .NET framework.

Are these tools that already exist for Bonsai.ML specifically? Or are you just speaking generally for the Bonsai ecosystem as a whole.

I'd rather we have the modern .NET support here deliberately based on need rather than having it sit around in an unknown/untested state.

(Also I'm pretty sure there's nothing stopping dotnet tools from being .NET Framework applications.)

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely have a wider discussion about this at some point because .NET Standard is meant as an alternative to multi-targeting. It rarely makes actual sense to use them together.

I agree, this is how I prefer to use it too, sadly the situation was forced on us by the problems with Rx.

We can decide to just break compatibility with all existing packages when Bonsai 3.0 arrives, but it's the general problems with ecosystems where if you do that we will be ditching all existing currently (possibly abandoned) packages which might still be useful.

My comments were general, but I do envisage using ML to run post-processing in Linux clusters via CLI. I can hack it via Mono but we will need some solution anyway. Harp suffered the same problem, where people were stuck not able to do self-contained apps with the library until I exposed a netstandard target.

I would be curious what was breaking in that case, I imagine it was related possibly to Mono but I don't know how that would be triggered given NuGet behavior unless there was some intermediate library targeting only netstandard also referencing these libs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PathogenDavid @glopesdev I think the issue you are referring to is this one: #31. I'm not sure if it is related to having multitargetted frameworks, but changing the torch.nn.Module derived classes to internal seemed to fix the issue for now. I've tested everything else on both Windows/Mono and it seems to work okay.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I somehow missed the way this was being done before. I was going to say we could have moved this to a separate PR, but seeing what is going on I think I agree we just ship it 😅

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 originally got attention because the ZipFile.ExtractToDirectory overload it used wasn't available. I thought there was also something subtly broken about the old implementation, but I can't remember what it was and I'm not seeing it now so maybe I just improved it while I was there.

This code is hopefully not long for this world anyway, Nick and I were talking about some more robust strategies for the future that won't involve downloading individual datasets all the time since this sort of thing is expected to become more common.

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