feat: add support for dependencies in requirements.txt with environment.yml #2883
Conversation
|
Exciting addition!! Let us know when it's ready for review or if you need input! |
|
Thanks @ruben-arts! I wasn't 100% sure at which stage would be best to handle the dependencies within the external requirements.txt. I'd love your feedback on this approach. Is there an issue with parsing the external dependencies as if they were directly in the environment.yml file? and if so, and what stage instead do you think those external dependencies should be handled? Also, I'd really appreciate some feedback on the error handling and if you think it's sufficient. Thanks for your help! |
| @@ -135,6 +144,26 @@ impl CondaEnvFile { | |||
| } | |||
| } | |||
|
|
|||
| fn get_deps_from_requirements_txt(indentation: &str, file_path: PathBuf) -> miette::Result<String> { | |||
There was a problem hiding this comment.
Would you be able to try to use: https://github.com/astral-sh/uv/blob/73cade13862c88bf2d5344759e9b1b7035b63743/crates/uv-requirements-txt/src/requirement.rs#L41
This might make the code more reliable and have better error messages in the long run.
| let base_path = path.parent().unwrap_or_else(|| Path::new("")); | ||
| let requirements_file_path = base_path.join(line_parts[1]); | ||
| let requirements_txt_pip_deps = | ||
| get_deps_from_requirements_txt(line_parts[0], requirements_file_path); |
There was a problem hiding this comment.
In theory, the best place to do the deserialization is in the CondaEnvFile struct. By writing a custom deserializer.
You could impl Deserialize for CondaEnvFile. But this is not a 10 minute refactor.
Otherwise, this location is okay given the current state of the code.
There was a problem hiding this comment.
Hey, question from a random guy who's invested in this feature shipping (looking for conda alternatives for ML projects but many open source ml projects still use requirements.txt): When you say using a custom deserializer, would that involve moving the all of from_path into the Deserialize impl, or just the codepath required for the requirements.txt additions?
There was a problem hiding this comment.
Yes indeed, on that implementation you could actually read the file and parse it.
|
Thanks for the feedback @ruben-arts! Will take a look at your suggestions and let you know once the PR is ready for another look. |
#1993