TypeScript support - Githubissues

lu4 commented 5 years ago

This PR is a major rewrite for the library, it introduces TypeScript support for node-opencl. Suggested changes are not backward compatible, there are changes in package.json, and changes in both C++ backend interface and NodeJS frontend interface, also MR contains couple of bugfixes, also there are changes in ... ... ... everywhere. I think this PR requires broader attention and some time to sync with existing code base. Or at least something to start with...

Hope to hear out your comments!

Thank you in advance!

lmeyerov commented 4 years ago

@lu4 - picking this back up

Does this preserve API compatibility with non-ts users? if not, can it be done in a way that does so, such as alt calls?

lu4 commented 4 years ago

Hi @lmeyerov, I'm having trouble with understanding what kind of compatibility are you asking about? Do you mean whether JavaScript developers would be able to use the provided api without the need of migrating to TypeScript? (If so than yes, they should have such ability, but this support might require couple of checks from my side and probably couple of updates. If this is the compatibility you are asking about then let me know)

@lu4 - picking this back up

Does this preserve API compatibility with non-ts users? if not, can it be done in a way that does so, such as alt calls?

lmeyerov commented 4 years ago

Yes, exactly:

-- can new node-opencl users use it without using typescript themselves? -- were there no public api changes, so existing node-opencl users can keep using it with no changes?

lmeyerov commented 4 years ago

If both are a yes, will help test + land soon after https://github.com/mikeseven/node-opencl/pull/69 . . If a no... bet we can figure it out :)

lu4 commented 4 years ago

-- can new node-opencl users use it without using typescript themselves? --> In theory yes, but appropriate checks / preparations should happen before the answer becomes a definitive yes

-- were there no public api changes, so existing node-opencl users can keep using it with no changes? --> It is hard for me to remember at this point all of the changes, but off-top of my head I've fixed couple of bugs I've found myself which in theory may break backward compatibility. As for public interface I've tried to stick as close as possible to both the original node-opencl design and official Khronos spec, and so the places where I've found severe discrepancy between the two, I've tried to improve (if such improvement was possible), these changes may also break the backward compatibility. The changes I remember are:

Int64 support (I made it little bit different, right now I don't remember how exactly I've implemented, but since then BigInt has became official part of ES standard which is better candidate than a pair of low / high numbers)
I've made deprecated OpenCL functions available (it shouldn't affect existing users, there's no need to be so restrictive about deprecated APIs)

Maybe there's something else, I don't remember. But what I can say for sure is that most of these changes would most probably affect major version of node-opencl.

In any case I'm open to help with transition (full or parts) but it'll definitely take some time to happen...

lmeyerov commented 4 years ago

Gotcha, thanks! We're seeing some fires on the new AWS G4s, which is ~4x cheaper than any of the others, so that shot up in priority. After we figure that out (1w?), will try this & report back!

lmeyerov commented 4 years ago

Hi @lu4 , just got commit access & merged the non-TS branch, and happy to test & land this one when you think it's ready.

lmeyerov commented 4 years ago

RE:Breaking changes, esp. for increased conformance & coverage , sounds worthwhile, we can just do a major version bump & call out in a changelog.

lu4 commented 4 years ago

@lmeyerov sounds cool, I'm experimenting with embind, ffi, genapi, nbind for other project these tools allow for automatic/semi-automatic bindings generation and look promising, but as I suspect we won't be throwing away existing code. So will signal you when ready, not promising anything, but hope to complete in couple of days

lmeyerov commented 4 years ago

Wow, went through all core files and most of tests, this is great!

AFAICT, this:

-- Adds TS 🚀🚀🚀 -- More nan upgrades 💪💪💪 -- Regresses on callback support? -- Disables a bunch of tests? 😔 -- Some conformance fixes: some arg orders, ... 💪💪 -- A few unnecessary refactors: CL_1_2 => v12, getInfo signature change around byte rep, ... -- Some file moves, some of which aren't version-tracked -- README/package.json changes from while this was a fork

My thinking to get landed: -- Need to document the breaking API-level changes. IMO, fine to do a least-effort solution like a migration section at the bottom of the main readme. -- May be smart to back out or split into a diff PR non-TS breaking changes ones such as the .info -- low value, mostly incurred work on others (e.g., we use that for some user-level scheduling, so annoying to port due to low value) -- What's going on w/ the event args? Looks like they got cut some reason? -- OK to lose version history on test/examples -- Tests should be reenabled -- I'm not a great reviewer of some of the C++ stuff, which compounds pain of a massive PR, but this is hard piecemeal so I get it :)

So maybe the flow is: -- Think about ^^^^ -- Address what is reasonable -- When ready, I can help basic AWS nvidia linux testing + osx intel -- Once we're happy with that, I can smoke test on our full production workloads -- ... and help with npm publishing (will figure out that flow w/ current branch) -- If the regressions get too hard to land immediately, we can start by doing a TS branch

Sounds reasonable / ideas?

Also, we can probably donate an Azure Pipelines CI testing across AWS intel/nvidia, if helpful

lmeyerov commented 4 years ago

@lmeyerov sounds cool, I'm experimenting with embind, ffi, genapi, nbind

Cool! Yeah, I'm cool with most things that eliminate code and improve maintainability. Some of those look like quite risky dependencies that'll bite us in 6mo so rather not take on the likely coming tech debt, but beyond that, am supportive.

lu4 commented 4 years ago

Ok, I'm back, will review and respond to your comments, will create a checklist and gradually will fix them...

lu4 commented 4 years ago

Ok, I've reviewed all your comments, I see that it might be too hard to try to move the new thing on top of old thing, there are too many layers of changes. Let's do it this way, I see that it's possible to introduce TypeScript bindings into existing project without touching the C++ side. I'll do this in a separate PR, and as of that we can start moving towards C++ changes (if necessary)

I'm saying "if necessary" because I'd rewrite the whole thing from scratch, I don't like most of the things I see, but unfortunately I don't have time for such adventure. So let's go forward with this step by step... What are your thoughts on this?

lmeyerov commented 4 years ago

Yes, splitting it up would make this a lot more approachable!

Maybe something like:

Prioritize an initial TS PR as a minor semvar bump: -- Make the TS match the existing signatures. If any breaking changes, make an explicit and well-motivated migration list -- I can help turn the list into an upgrade guide + help w/ device & integration testing -- => Most people can start using with high confidence and upgrade with little pain

2a. C++ changes (nan, ...) as a diff PR, ideally also as a minor semvar that conforms to (1)'s TS => same benefits as (1), and easier to do the security etc. review

2b. New abstractions (eg., async/await) as a major semvar, and maybe have live as an @next for awhile => "node-opencl 2.0"

EDIT: Should I respond to the comments, or do anything else to help for next steps?

mikeseven / node-opencl

TypeScript support #70