Infra suggestions #2

MichelML · 2022-04-28T19:11:49Z

I was curious about this project so I went on and built it locally. I found the interactions very interesting and useful.

To build the project I ran into a few problems, and had a few ideas regarding "infra stuff". See more of my inline comments below.

I'm happy to help further if you have other things you'd like to add in this project.

A few other things, I wonder how it could fit with my current attempt to have a dedicated rdkit-js repository in the GitHub rdkit org. Your examples are very useful here, I wonder if this could be included somewhere with other examples seen in https://rdkitjs.com/

A little more context here rdkit/rdkit#4124

rdkit-js npm dependency and fix pika

MichelML · 2022-04-28T19:14:23Z

.github/dependabot.yml

+  directory: "/"
+  schedule:
+    interval: weekly
+  open-pull-requests-limit: 20


The file above is a dependabot file making automatic pull-requests based on dependencies that should/could be updated

more about this https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#package-ecosystem

MichelML · 2022-04-28T19:15:09Z

.gitignore

@@ -1,3 +1,5 @@
 node_modules
 dist
 RDKitStructureRenderer-*.tgz
+pkg
+public/RDKit_minimal.*


pkg and the RDKit_minimal files are git ignored since they are managed by the build process during npm run build now

MichelML · 2022-04-28T19:17:17Z

gulpfile.js

+  ).pipe(dest("public/"));
+}
+
+exports.rdkitCopy = rdkitCopy;


this is a gulp task copying the RDKit minimal lib files (js and wasm files) to the public folder (the initial place they were in before this PR, only now these files will always mirror the version of rdkit-js you have installed in your local repository).

MichelML · 2022-04-28T19:19:42Z

package.json

@@ -8,7 +8,7 @@
        "build": "npm run build:pkg && npm run build:prod",
        "build:dev": "webpack --mode=development",
        "build:prod": "webpack --mode=production --node-env=production",
-        "build:pkg": "pika build",
+        "build:pkg": "gulp rdkitCopy && pika-pack",


gulp rdkitCopy relates to the task defined in https://github.com/rdkit/rdkit-structure-renderer/pull/2/files#diff-25789e3ba4c2adf4a68996260eb693a441b4a834c38b76167a120f0b51b969f7

Regading the pika build --> pika-pack change, I noticed that the recent version of the package @pika/pack offers only pika-pack as a bin command. Prior to this change, I was getting a pika not found error.

I'm using npm version 8.8.0

MichelML · 2022-04-28T19:21:12Z

package.json

@@ -40,10 +40,12 @@
        "@pika/plugin-bundle-web": "^0.5.0",
        "@pika/plugin-copy-assets": "^0.5.0",
        "@pika/plugin-standard-pkg": "^0.5.0",
+        "@rdkit/rdkit": "^2022.3.2",


The new setup is using the npm release of the JavaScript rdkit MinimalLib https://www.npmjs.com/package/@rdkit/rdkit

MichelML · 2022-04-28T19:22:04Z

package.json

        "@webpack-cli/generators": "^2.4.2",
        "babel-loader": "^8.2.3",
        "eslint": "^8.10.0",
        "eslint-webpack-plugin": "^3.1.1",
+        "gulp": "^4.0.2",


a necessary dev dependency for the new npm run build command to work properly

MichelML · 2022-04-28T22:26:30Z

gulpfile.js

+}
+
+exports.rdkitCopy = rdkitCopy;
+exports.packageProject = packageProject;


This is another gulp command doing what the previous npm run build:pkg was doing

MichelML · 2022-06-02T15:15:00Z

bump on this @ptosco , I saw that you were updating the wasm file by hand, I think using the npm release would be more convenient (and the version of the minimal lib used by this repo would be tracable in the package.json file), but I'm biased and you're obviously the master of this project 🙏

If you want, I can add you to the npm @rdkit organization for you to have a better control over the npm releases too

ptosco · 2022-06-03T09:02:14Z

@MichelML Sorry for being slow on this one. I agree that tying this to the upstream repo would be tidier, but it often happens that I need functionality which has already been merged into master but not yet released (e.g. in this case returning the indices mapping when abbreviations are toggled), so I anticipate I'll often need to bundle a "preview" copy.
I'll try and come up with a better solution, and also to find the time to properly review and merge this PR - please bear with me as I have many other things on my plate right now.

MichelML · 2022-06-03T14:48:52Z

No worries @ptosco , all my comments (and this PR) are assuming that eventually you'll want to release this as an npm package.

If this is the case, having a development setup allowing you to do what you describe in your last comment is a good thing to have for sure, but always tying official release of this package with an official release of rdkit when publishing on npm would also be a must.

Going back to npm releases of the minimal lib , in between releases, you could publish beta versions of the minimal lib like so - although I would have to invite you to the @rdkit npm org (Greg and I are the only members at the moment).

Where if you're preparing this package for an upcoming release of rdkit, you could name your beta versions similar to 2022.3.3-next or 2022.4.3-beta or 2022.3.3-next-${GITHUB_SHA}

Just some ideas 🙏

MichelML · 2022-06-03T15:35:58Z

Update: just saw it was released already https://www.npmjs.com/package/rdkit-structure-renderer

ideally it should be published under @rdkit too 🙏 (the name in package.json should thus be @rdkit/rdkit-structure-renderer ), if you give me your email address @ptosco I'll send you an invite to the org

MichelML · 2022-07-08T19:15:00Z

@ptosco if I find the time to do so in the upcoming weeks, would you feel comfortable putting this project under https://github.com/rdkit/rdkit-js ?

I would take the time to setup the npm release process for this package and the deployment of the website that comes with it as well. Your workflow and the structure of this project wouldn't change except that you would work under a specific folder under rdkit-js basically

your package would still be your own, released as @rdkit/structure-renderer on npm , I'm thinking of the following structure going forward (mid-term) for npm packages related to minimal lib (yet it would be a multi-packages mono-repo all under rdkit/rdkit-js :

@rdkit/rdkit-js (or could be renamed as @rdkit/core)
@rdkit/structure-renderer (this current project, using @rdkit/rdkit-js as a dependency)
@rdkit/react-components (react.js components including a react-based renderer [something I'm working on at the moment], that could eventually use your renderer under the hood)

curious to have your thoughts on this 🙏

ptosco · 2022-07-20T15:33:33Z

@MichelML I think that should be possible. I already have a React component based on rdkit-structure-renderer that I might be able to contribute. I'll get back to you before the end of August.

MichelML · 2022-07-20T15:43:15Z

@ptosco awesome! looking forward to it!

MichelML · 2022-10-07T18:12:24Z

Closing here since I don't think this will get merged, feel free to reopen anytime, happy to contribute to the project in the future if you need help @ptosco

MichelML added 4 commits April 28, 2022 14:52

use rdkit-js npm release

99ed7cf

add dependabot

66b3fca

rdkit-js npm dependency and fix pika

do not include rdkit minimal in versioning

d3523fd

add missing gulp dev dep

1e71828

MichelML commented Apr 28, 2022

View reviewed changes

MichelML mentioned this pull request Apr 28, 2022

MinimalLib: Bug in aromatic systems (both with kekulize and not) rdkit/rdkit#5244

Closed

build package entirely with gulp for cross-shell/os compatibility

cf82bf1

MichelML commented Apr 28, 2022

View reviewed changes

MichelML added 2 commits June 2, 2022 11:12

fix conflicts

bba5ba5

bump rdkit version

dff0a77

MichelML mentioned this pull request Jul 20, 2022

Remove obsolete files related to rdkit-js + write a rescoped README for MinimalLib rdkit/rdkit#5432

Merged

MichelML closed this Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infra suggestions #2

Infra suggestions #2

MichelML commented Apr 28, 2022

MichelML Apr 28, 2022

MichelML Apr 28, 2022

MichelML Apr 28, 2022 •

edited

MichelML Apr 28, 2022 •

edited

MichelML Apr 28, 2022

MichelML Apr 28, 2022

MichelML Apr 28, 2022

MichelML commented Jun 2, 2022 •

edited

ptosco commented Jun 3, 2022

MichelML commented Jun 3, 2022 •

edited

MichelML commented Jun 3, 2022 •

edited

MichelML commented Jul 8, 2022 •

edited

ptosco commented Jul 20, 2022

MichelML commented Jul 20, 2022

MichelML commented Oct 7, 2022

Infra suggestions #2

Infra suggestions #2

Conversation

MichelML commented Apr 28, 2022

MichelML Apr 28, 2022

Choose a reason for hiding this comment

MichelML Apr 28, 2022

Choose a reason for hiding this comment

MichelML Apr 28, 2022 • edited

Choose a reason for hiding this comment

MichelML Apr 28, 2022 • edited

Choose a reason for hiding this comment

MichelML Apr 28, 2022

Choose a reason for hiding this comment

MichelML Apr 28, 2022

Choose a reason for hiding this comment

MichelML Apr 28, 2022

Choose a reason for hiding this comment

MichelML commented Jun 2, 2022 • edited

ptosco commented Jun 3, 2022

MichelML commented Jun 3, 2022 • edited

MichelML commented Jun 3, 2022 • edited

MichelML commented Jul 8, 2022 • edited

ptosco commented Jul 20, 2022

MichelML commented Jul 20, 2022

MichelML commented Oct 7, 2022

MichelML Apr 28, 2022 •

edited

MichelML Apr 28, 2022 •

edited

MichelML commented Jun 2, 2022 •

edited

MichelML commented Jun 3, 2022 •

edited

MichelML commented Jun 3, 2022 •

edited

MichelML commented Jul 8, 2022 •

edited