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

Rename to coglet plus reorg #5

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Rename to coglet plus reorg #5

merged 3 commits into from
Nov 18, 2024

Conversation

meatballhat
Copy link
Contributor

@meatballhat meatballhat commented Nov 17, 2024

so that:

  • python -m coglet is the main entrypoint
  • cog compatibility imports are a thin wrapper around coglet.api and are only available for import once coglet/_compat is put into sys.path thus greatly reducing the likelihood of import collisions.
  • cog/internal/ directory collapsed up a level to coglet/ the above import collision problem is fixed.

so that:

- `python -m coglet` is the main entrypoint
- `cog` compatibility imports are a thin wrapper around `coglet.api` and
  are only available for import once `coglet/_compat` is put into
`sys.path`, thus greatly reducing the likelihood of import collisions.
- `cog/internal/` directory collapsed up a level to `coglet/` since the
  above import collision problem is fixed.
@meatballhat meatballhat requested a review from a team as a code owner November 17, 2024 20:16
Copy link
Contributor

@nevillelyh nevillelyh left a comment

Choose a reason for hiding this comment

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

LGTM, mostly question w.r.t. the assumption of this being runtime only.

python/coglet/__init__.py Outdated Show resolved Hide resolved
category=ImportWarning,
stacklevel=2,
)
sys.path.insert(0, str(pathlib.Path(__file__).absolute().parent / '_compat'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also assuming coglet is only injected at runtime right? I was hoping that, down the road, this would replace vanilla in development, i.e. cog build, so we can fast fail if user does something funny. This means we'll have to install both coggo & coglet in user image and:

  • validate schema with the inspector
  • check prediction works through coggo -> coglet.file_runner
  • check user code imports only cog.* and nothing internal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like what I have in mind, yes. I'd want to maintain this repository as cog-runtime, which includes a coggo/cog-server and coglet as a separate project from Cog, then inject them at image build time for typical development use, or via Monobase for fast push. This would mean that we'd be able to have a transition period where we can decide to install either the legacy Cog runtime wheel or this project's Go binary and wheel, and eventually remove all of the ./python tree from Cog.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. And we can drop the _compat alias once there's only 1 impl.
Do we want to use coggo/coglet in fast push cog build time? Ideally yes if we always use them at runtime, so that we can detect breakage before user pushing. But that also depends how much feature parity we get before releasing fast push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to introduce a way to inject coggo/coglet via the fast push cog build soon, yes 👍🏼 Exactly your point about detecting breakage before user push.

Base automatically changed from python-matrix to main November 18, 2024 01:53
@meatballhat meatballhat merged commit dcf6749 into main Nov 18, 2024
11 checks passed
@meatballhat meatballhat deleted the coglet branch November 18, 2024 12:27
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.

2 participants