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

requirement and pyproject.toml sync checking #1141

Closed
wants to merge 1 commit into from

Conversation

Dhruv-Sachdev1313
Copy link

Description of changes

Summarize the changes made by this PR.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

@github-actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

@Dhruv-Sachdev1313
Copy link
Author

Dhruv-Sachdev1313 commented Sep 13, 2023

I am yet to add a GitHub Workflow to make it a part of every CI process. Before I do that I just wanted to make sure if this script is fine in terms of the issue (Script works for sure!), hence the Draft PR.
@levand @jeffchuber @HammadB
Please change the Base branch, I was not sure which branch to make a PR against

@tazarov
Copy link
Contributor

tazarov commented Sep 14, 2023

@Dhruv-Sachdev1313, thank you for the script. Do you think it makes sense to step back for a second and understand the historical reason for having separate pyproject.toml and requirements.txt?

Without knowing the history, I can only deduce that requirements.txt is just a reflection of pyproject.toml's dependencies section. If that is the case, I wonder if it also makes sense to have instead a single source of truth that lies with pyproject.toml and generate requirements.txt from it as needed.

@Dhruv-Sachdev1313
Copy link
Author

Dhruv-Sachdev1313 commented Sep 14, 2023

@Dhruv-Sachdev1313, thank you for the script. Do you think it makes sense to step back for a second and understand the historical reason for having separate pyproject.toml and requirements.txt?

Without knowing the history, I can only deduce that requirements.txt is just a reflection of pyproject.toml's dependencies section. If that is the case, I wonder if it also makes sense to have instead a single source of truth that lies with pyproject.toml and generate requirements.txt from it as needed.

Hey @tazarov: I was wondering about it as well, I am new to the repository and found an issue stating this(Ensure pyproject.toml is correct as part of CI for PRs. #564 ) It said, The dependencies in requirements.txt and pyproject.toml must match in order for the project to work correctly in development and when deployed.
Now there might be a good chance that I misunderstood the objective of the issue. If that is so could you please help me out here?

@tazarov
Copy link
Contributor

tazarov commented Sep 14, 2023

Hey @tazarov: I was wondering about it as well, I am new to repository and found an issue stating this(Ensure pyproject.toml is correct as part of CI for PRs. #564 ) It said, The dependencies in requirements.txt and pyproject.toml must match in order for the project to work correctly in development and when deployed.
Now there might be a good chance that I misunderstood the objective to the issue. If that is so could you please help me out here?

Hey @Dhruv-Sachdev1313, actually, I went through to #564, and I can see your understanding is correct. I am wondering, though, whether there is a more subtle way to solve the problem at hand or just reduce the problem to maintaining a single dependency graph rather than trying to maintain and synchronize two, which can lead to the original problem of this issue.

@HammadB, wdyt? Any idea why we would need two sources of dependencies?

@jeffchuber
Copy link
Contributor

@Dhruv-Sachdev1313 @tazarov doing some cleanup here - is this something we still want to merge? want to close?

@tazarov
Copy link
Contributor

tazarov commented Jan 2, 2024

@jeffchuber, I think it'd be foolish to ignore that out-of-sync dependencies between pyproject.toml and requirements.txt have and can cause issues for users. I think it would be wise to consider the sync problem; I am unsure if using the script in its current form is the right solution.

We can either close this PR and spend a moment to think about the problem of sync and perhaps have a well-reasoned CIP to back any choices made, or we can accept the PR as is and build upon it, knowing that this is a step in the right direction.

I am okay with either approach.

@tazarov tazarov linked an issue Jun 18, 2024 that may be closed by this pull request
@jeffchuber
Copy link
Contributor

Unfortunately this is quite out of date (due to no fault of the OP!) - for hygiene I am going to close this now.

@jeffchuber jeffchuber closed this Sep 15, 2024
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.

[ENH] Cleanup, lint, and sync requirements
3 participants