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

feat: Dev Container for consistent dev setup #5143

Closed

Conversation

maryamtahhan
Copy link
Contributor

@maryamtahhan maryamtahhan commented Nov 14, 2024

Added a Dev Container configuration to streamline development and onboarding. This setup ensures a consistent, isolated environment with all necessary tools and dependencies for building and running Triton.

Edit: I updated the Dev Container to enable the LLVM build based on a flag in devcontainer.json rather than editing the Dockerfile directly. This should lower the barrier to entry for using containers for new users. Additionally the LLVM build now pulls the commit id from cmake/llvm-hash.txt or the user can edit the COMMIT directly.

New contributor declaration

  • I am not making a trivial change, such as fixing a typo in a comment.

  • I have written a PR description following these
    rules.

  • I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • I have added tests.
      • /test for lit tests
      • /unittest for C++ tests
      • /python/test for end-to-end tests
    • This PR does not need a test because it's to do with the developer workflow and onboarding rather than a code change.
  • Select one of the following.

    • I have not added any lit tests.
    • The lit tests I have added follow these best practices,
      including the "tests should be minimal" section. (Usually running Python code
      and using the instructions it generates is not minimal.)

Copy link
Collaborator

@ThomasRaoux ThomasRaoux left a comment

Choose a reason for hiding this comment

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

This is likely to quickly get outdated. I think this may be a bit out of scope for this repo. Could you find a better place for this? And maybe we can have a link for it in our readme

@maryamtahhan
Copy link
Contributor Author

Thank you @ThomasRaoux for your feedback.
Could you clarify your concern about the dev container potentially becoming outdated quickly?

Also, could you elaborate on why improving the onboarding experience for developers might be considered out of scope?

The purpose of including the dev container in the repository is to provide a consistent development environment across various setups. This consistency benefits both new developers during onboarding and existing contributors. If the dev container is part of the repo, any updates to the project are automatically reflected in the dev container immediately.

@maryamtahhan maryamtahhan force-pushed the triton-devcontainer branch 3 times, most recently from d2760bf to 68f47d1 Compare November 20, 2024 09:50
@ThomasRaoux
Copy link
Collaborator

Thank you @ThomasRaoux for your feedback. Could you clarify your concern about the dev container potentially becoming outdated quickly?

Also, could you elaborate on why improving the onboarding experience for developers might be considered out of scope?

The purpose of including the dev container in the repository is to provide a consistent development environment across various setups. This consistency benefits both new developers during onboarding and existing contributors. If the dev container is part of the repo, any updates to the project are automatically reflected in the dev container immediately.

sorry for the slow response. My concerns is that when something change in the flow those files are not going to be updated and will be staled.

@maryamtahhan
Copy link
Contributor Author

sorry for the slow response. My concerns is that when something change in the flow those files are not going to be updated and will be staled.

No problem. How frequently does the Triton flow change? The dev container files typically need updates only when there’s a significant change in tooling or dependencies.
To address the concern, we can document the purpose and scope of these files in the CONTRIBUTING.md file, explicitly stating when they need to be reviewed (e.g., during changes to dependencies or tooling). Additionally/alternatively, we can include a checklist item in PR templates to remind contributors to verify the dev container files or update them when dependency/tooling changes are made.

To further mitigate concerns, I’d be happy to take responsibility for updating these files if the development environment changes significantly.

Even if these files aren’t updated immediately with every future change, they won’t negatively impact the core application. At worst, they might require minor adjustments later, but their inclusion now ensures a more consistent developer experience..

Added a Dev Container configuration to streamline
development and onboarding. This setup ensures a
consistent, isolated environment with all necessary
tools and dependencies for building and running
Triton.

Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Improve the Dockerfile to separate out the LLVM
build stage and trigger the build through a simple
argument in `devcontainer.json` to lower the barrier
to entry to devcontainer use.

Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan
Copy link
Contributor Author

@ThomasRaoux just wondering if you had a chance to consider my last response?

Would Triton consider adding this as a git submodule if it lived in another repo? it's most useful when it lives in the actual repo, but a submodule could be a reasonable compromise?

@ThomasRaoux
Copy link
Collaborator

@ThomasRaoux just wondering if you had a chance to consider my last response?

Would Triton consider adding this as a git submodule if it lived in another repo? it's most useful when it lives in the actual repo, but a submodule could be a reasonable compromise?

sorry for the slow response as I was away. I think submodule is too heavy for this. It's hard for me to get a feel of how much this helps the community without knowing if people would use it.
I think to start with we could have some update to the readme and a link to some setup but I'd prefer not land this at moment.

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