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

Remove dependency file generator from checks #46

Open
vyasr opened this issue Feb 21, 2023 · 14 comments
Open

Remove dependency file generator from checks #46

vyasr opened this issue Feb 21, 2023 · 14 comments

Comments

@vyasr
Copy link
Contributor

vyasr commented Feb 21, 2023

Currently the checks shared workflow includes a job that runs dfg. As of version 1.2, dfg supports being run as a pre-commit hook, which is superior to this workflow because it can be run identically both locally and in CI. Moreover, all RAPIDS repos were migrated to using pre-commit for style checks during the GHA migration, so there's very little additional work required to add the new dfg hook. Once all repositories have been migrated to use the hook, the dfg support can be removed from the check here.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 21, 2023

CC @ajschmidt8

@ajschmidt8
Copy link
Member

@vyasr, do we have plans to adapt this hook to support the commands below, which remove any existing generated files before re-running the dfg tool?

These lines ensure that files that are no longer generated by the tool as cleaned up from the repository.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 22, 2023

Yeah I realized that was a missing bit. We could implement that as part of the Python package itself; I think it would be reasonable to do and better than relying on the action for this. The main question would be where we start searching for these files from. Since the location of dependencies.yaml is configurable, I assume that we don't want to search from the directory containing dependencies.yaml. The action assumes that it is running from the root of the repository. Do we want to bake the assumption that we are in a git repository into dfg? If so, we could use GitPython.

@ajschmidt8
Copy link
Member

The action assumes that it is running from the root of the repository. Do we want to bake the assumption that we are in a git repository into dfg? If so, we could use GitPython.

I'd really like to avoid adding additional dependencies to dfg if possible.

How does pre-commit handle this?

Can pre-commit be configured to run it from the root repository without needing GitPython installed?

Or will it execute from the current working directory (which may not be the root of the repository)?

I think starting from the location of dependencies.yaml is probably a fine idea since the generated files are all written to paths relative to that source file (and presumably, though not guaranteed, not written to directories higher up in the file tree. e.g. ../../some/path).

@vyasr
Copy link
Contributor Author

vyasr commented Feb 22, 2023

That's a good point, pre-commit will always run from the root of the repository so we should be able to just use that directly. I'll prototype that solution.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 23, 2023

rapidsai/dependency-file-generator#32 adds the necessary feature to dfg. Once that is in, we should be able to specify the --clean option to the pre-commit hooks in each repo and move forward with the removal proposed here.

vyasr added a commit to rapidsai/dependency-file-generator that referenced this issue Feb 23, 2023
…32)

This adds the ability to request that dfg erase any files it previously
created before generating new ones. This logic may be used to replace
cleanup logic already present in the tests as well as logic currently
embedded [in the GHA tool currently running
dfg](https://github.com/rapidsai/gha-tools/blob/37f3946294647eadf5b60d8229ac396a4d913b59/tools/rapids-dependency-file-checker#L9-L15).
Moving this logic directly into dfg will allow us to use its pre-commit
hook as the primary source of truth for how to run it, both locally and
in CI.

Contributes to
rapidsai/shared-workflows#46

---------

Co-authored-by: AJ Schmidt <ajschmidt8@users.noreply.github.com>
@bdice
Copy link
Contributor

bdice commented May 30, 2023

Repos that still have this check enabled:

  • dask-cuda
  • cucim
  • kvikio
  • cuxfilter
  • ucx-py
  • rapids-cmake
  • some other private repos

@bdice
Copy link
Contributor

bdice commented May 30, 2023

I cleaned this up a bit in #102 but we need to address the repos listed above.

@jakirkham
Copy link
Member

Would it make sense to move this issue to build-planning?

@jameslamb
Copy link
Member

jameslamb commented Jan 26, 2024

Looked at all the non-archived repos in the rapidsai org with pre-commit configs today (GitHub search).

Of those, the following had a dependencies.yaml at the root of the repo but no pre-commit check running rapids-dependency-file-generator.

@vyasr
Copy link
Contributor Author

vyasr commented Jan 29, 2024

Thanks for getting started with this. It would be nice to clean up the remainder of these when you have some bandwidth!

rapids-bot bot pushed a commit to rapidsai/rapids-cmake that referenced this issue Feb 5, 2024
Contributes to rapidsai/shared-workflows#46.

Proposes adding a pre-commit hook to run `rapids-dependency-file-generator`.

With that change, local `pre-commit` and CI will raise an error if changes to `dependencies.yaml` cause some error from `rapids-dependency-file-generator`.

If this project checks any files modified by that tool into source control in the future, it'd also raise an error if any of those files change.

This is a bit cheaper way to find out about things like typos, misaligning spacing, etc. than waiting for CI to report it. It's very fast, shouldn't be noticeable in day-to-day development.

### How I tested this

Changed an `output: conda` to `output: condafile` (not a valid output type) in `dependencies.yaml`.

Ran `pre-commit run --all-files`.

Observed the expected failure.

<img width="852" alt="Screenshot 2024-01-31 at 9 14 10 AM" src="https://github.com/rapidsai/rapids-cmake/assets/7608904/bd8dc1d6-8e5c-424c-b493-6fd6d5d10dce">

Reverted that change, ran `pre-commit run --all-files` again, saw everything pass.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #531
@vyasr
Copy link
Contributor Author

vyasr commented Mar 27, 2024

Based on the comments above, I think we can disable the check now in shared-workflows but leave the option as a no-op while we go through every repo removing the option, then once we're confident everything has it removed we can remove the check entirely from here. Am I missing anything?

@jameslamb
Copy link
Member

I agree with that @vyasr

@vyasr vyasr mentioned this issue Mar 27, 2024
@vyasr
Copy link
Contributor Author

vyasr commented Mar 27, 2024

Removing in #204

difyrrwrzd added a commit to difyrrwrzd/dependency-file-generator that referenced this issue Aug 10, 2024
…(#32)

This adds the ability to request that dfg erase any files it previously
created before generating new ones. This logic may be used to replace
cleanup logic already present in the tests as well as logic currently
embedded [in the GHA tool currently running
dfg](https://github.com/rapidsai/gha-tools/blob/37f3946294647eadf5b60d8229ac396a4d913b59/tools/rapids-dependency-file-checker#L9-L15).
Moving this logic directly into dfg will allow us to use its pre-commit
hook as the primary source of truth for how to run it, both locally and
in CI.

Contributes to
rapidsai/shared-workflows#46

---------

Co-authored-by: AJ Schmidt <ajschmidt8@users.noreply.github.com>
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

No branches or pull requests

5 participants