Skip to content

More refined dependency management for different app types#70

Merged
bradhe merged 15 commits intodevelopfrom
tasks/skip-sync-when-missing-pyproject.toml
Jul 21, 2025
Merged

More refined dependency management for different app types#70
bradhe merged 15 commits intodevelopfrom
tasks/skip-sync-when-missing-pyproject.toml

Conversation

@bradhe
Copy link
Contributor

@bradhe bradhe commented Jul 20, 2025

This PR does a few different things.

  1. Always setup a virtual environment for every app.
  2. If the user is using requirements.txt (our legacy suggestion), then manage dependencies using that.
  3. If the user is using pyproject.toml, use uv sync for setup.
  4. If the user is using neither, do nothing.

It also adds some test coverage for running different types of apps via tower-runtime to ensure the behavior doesn't change over time.

@bradhe bradhe requested review from Copilot and konstantinoscs July 20, 2025 10:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds error handling for missing pyproject.toml files in the uv sync operation and introduces basic regression tests for the tower-runtime crate. The main change skips the sync operation when no pyproject.toml is present rather than allowing uv to fail.

  • Adds early detection and graceful handling of missing pyproject.toml files
  • Introduces comprehensive test coverage for LocalApp runtime with two example scenarios
  • Updates Status enum to support equality comparisons needed for testing

Reviewed Changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/tower-uv/src/lib.rs Adds MissingPyprojectToml error variant and pre-sync validation
crates/tower-runtime/src/local.rs Updates sync logic to gracefully handle missing pyproject.toml
crates/tower-runtime/src/lib.rs Adds PartialEq trait to Status enum for test assertions
crates/tower-runtime/src/errors.rs Maps new MissingPyprojectToml error to SpawnFailed
crates/tower-runtime/tests/local_test.rs Adds comprehensive test suite with example apps
crates/tower-runtime/Cargo.toml Adds config dev dependency for testing
Comments suppressed due to low confidence (2)

crates/tower-runtime/tests/local_test.rs:71

  • The error message 'App should be running' is incorrect for an assertion that checks if status equals Status::Exited. It should be 'App should be exited' or 'App should have exited'.
    assert!(status == Status::Exited, "App should be running");

crates/tower-runtime/tests/local_test.rs:123

  • The error message 'App should be running' is incorrect for an assertion that checks if status equals Status::Exited. It should be 'App should be exited' or 'App should have exited'.
    assert!(status == Status::Exited, "App should be running");

tower_uv::Error::NotFound(_) => Error::SpawnFailed,
tower_uv::Error::PermissionDenied(_) => Error::SpawnFailed,
tower_uv::Error::Other(_) => Error::SpawnFailed,
tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed,
Copy link

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

The indentation is inconsistent with the surrounding code. The line should be indented to align with the other match arms above it.

Suggested change
tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed,
tower_uv::Error::MissingPyprojectToml => Error::SpawnFailed,

Copilot uses AI. Check for mistakes.
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 don't understand this one? Maybe there's something messed up in my local enviro config.

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it just wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be to me? Thought maybe it was some hidden whitespace issue perhaps.

@bradhe bradhe requested a review from socksy July 20, 2025 14:36
@bradhe bradhe changed the title Skip sync when missing pyproject.toml More refined dependency management for different app types Jul 21, 2025
Comment on lines +79 to +80
let (sender, receiver) = unbounded_channel::<Output>();

let output_sender = Arc::new(Mutex::new(sender));
let output_receiver = Arc::new(Mutex::new(receiver));
(output_sender, output_receiver)
(sender, receiver)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. You can elide the let binding completely now, so just calling unbounded_channel
  2. If that's the case, you could possibly elide this function completely?
  3. Is there a reason we don't use a bounded channel? Unbounded channels use a linkedlist. I presume if we have a large enough bounded channel we're unlikely to ever get backpressure, and it may improve latency due to being friendlier to cpu cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right about eliding the let binding, I think we can do that. The only purpose of this function is to manage the creation of our internal type, OutputSender and OutputReceiver. This kind of "hides" the fact that we use tokio under the hood.

Anyway, we might be able to push all this on to the user. I'd say lets experiment with that a bit later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you were right about eliding this whole thing away. Nice.

Comment on lines +150 to +153
match test_uv_path(&self.uv_path).await {
Ok(_) => true,
Err(_) => false,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

untested, but can probably do this?

Suggested change
match test_uv_path(&self.uv_path).await {
Ok(_) => true,
Err(_) => false,
}
(&self.uv_path).await.is_ok()

Copy link
Contributor

@socksy socksy left a comment

Choose a reason for hiding this comment

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

LGTM, and the core logic doesn't seem to break when I throw some projects with requirements.txt at it etc? not 100% sure if I effectively tested it though 😅

@bradhe bradhe merged commit 79cbe85 into develop Jul 21, 2025
4 checks passed
@bradhe bradhe deleted the tasks/skip-sync-when-missing-pyproject.toml branch July 21, 2025 13:57
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