Skip to content

Import Conductor as package#66

Merged
loyaltypollution merged 7 commits intosource-academy:mainfrom
loyaltypollution:refactor-conductor
Oct 17, 2025
Merged

Import Conductor as package#66
loyaltypollution merged 7 commits intosource-academy:mainfrom
loyaltypollution:refactor-conductor

Conversation

@loyaltypollution
Copy link
Contributor

Previously, py-slang copied the Conductor framework code directly into the repo. This PR replaces that approach by consuming Conductor as an npm library instead.

I worked with @tsammeow to expose the correct library API for py-slang to import from.
This change significantly reduces the codebase size of py-slang while keeping functionality the same.

  • Removes duplicated Conductor code in py-slang
  • Centralizes Conductor maintenance in its own package
  • Decreases codebase size

@s-kybound
Copy link
Member

I really like the direction this PR is taking with the conductor - it should be standardized as a package instead of being copied across all of the evaluators. will take a closer look the PR.

@martin-henz
Copy link
Member

Note that we are trying to adhere to these new conventions for Source Academy projects: https://github.com/source-academy/general/wiki/Conventions-and-practices

If there is a need for discussion of those, let's use issues in the repo "general".

Copy link
Contributor

@WangYuyang1013 WangYuyang1013 left a comment

Choose a reason for hiding this comment

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

This PR takes a very good direction, and I appreciate the effort to reduce duplication and standardize the use of Conductor as a package. While I may not fully understand all the code changes in detail, I trust that if it works correctly on your side, it should be fine.

@s-kybound
Copy link
Member

hold on the merge for a bit!

Copy link
Member

@s-kybound s-kybound left a comment

Choose a reason for hiding this comment

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

again, a resounding yes for this direction. but is conductor not meant to be imported as a NPM library @sourceacademy/conductor, and not a git repo?

cc @tsammeow

once resolved, LGTM

@loyaltypollution
Copy link
Contributor Author

I'm looking at frontend and other repos like this, it seems like we are currently using tagged github repositories for all package.json.

Is it because our work is still in development? @tsammeow @s-kybound

@martin-henz
Copy link
Member

I'm looking at frontend and other repos like this, it seems like we are currently using tagged github repositories for all package.json.

Is it because our work is still in development? @tsammeow @s-kybound

Yes, let's use NPM for such dependencies. We have a few packages in NPM with prefix sourceacademy already: https://www.npmjs.com/search?q=sourceacademy
...and of course js-slang is an NPM package as well:
https://www.npmjs.com/package/js-slang

Let's follow our guide on this:

https://github.com/source-academy/general/wiki/Conventions-and-practices

Copy link
Contributor

@WangYuyang1013 WangYuyang1013 left a comment

Choose a reason for hiding this comment

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

This seems better. It was a bit messy when it was originally placed in types.

@loyaltypollution loyaltypollution merged commit a6b2abc into source-academy:main Oct 17, 2025
1 check passed
@loyaltypollution loyaltypollution deleted the refactor-conductor branch January 28, 2026 15:58
loyaltypollution added a commit to loyaltypollution/py-slang that referenced this pull request Mar 14, 2026
* Move conductor code to npm package

* Use conductor submodules

* Use source-academy/conductor 0.2.2

* Use npm based conductor

* Fix bundler issues

* Fix circular imports

* Fix circular imports (types)
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.

4 participants