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

Add fin as a submodule #1588

Merged
merged 15 commits into from
Feb 3, 2023
Merged

Add fin as a submodule #1588

merged 15 commits into from
Feb 3, 2023

Conversation

JehandadKhan
Copy link
Contributor

This PR removes the old fin code which was added verbatim and replaces it with a git submodule. This allows better tracking with the upstream repo which is developed independently.

@JehandadKhan
Copy link
Contributor Author

In draft state until I verify behavior of Jenkins with git submodules.

@pfultz2
Copy link
Contributor

pfultz2 commented Jun 8, 2022

Why is fin developed independently? If it needs to be then why doesn't MIOpen use fin with find_package?

Or is fin more tightly coupled with MIOpen that it needs to build with MIOpen? If so, then why is this in a separate repo?

@junliume
Copy link
Contributor

junliume commented Jun 9, 2022

@JehandadKhan "Tuna Fin Build Test" stage has failed, and I guess that's expected?

@JehandadKhan JehandadKhan marked this pull request as ready for review June 16, 2022 19:16
@JehandadKhan JehandadKhan marked this pull request as draft June 30, 2022 16:02
@cderb cderb self-assigned this Feb 1, 2023
@cderb cderb requested a review from junliume February 1, 2023 17:31
@cderb cderb requested a review from averinevg February 1, 2023 17:31
@cderb cderb marked this pull request as ready for review February 1, 2023 17:32
@cderb
Copy link
Contributor

cderb commented Feb 1, 2023

Completed work with converting Fin to a submodule.

@JehandadKhan
Copy link
Contributor Author

@cderb @junliume I am unable to approve this PR, since I am the original author.

@cderb
Copy link
Contributor

cderb commented Feb 3, 2023

@cderb @junliume I am unable to approve this PR, since I am the original author.

Curiously enough I can approve it even though I was the last to make changes.

Copy link
Contributor

@junliume junliume left a comment

Choose a reason for hiding this comment

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

@JehandadKhan it seems to be no functional changes except impacts from Jenkins file on CI?

@cderb
Copy link
Contributor

cderb commented Feb 3, 2023

@junliume This removes the Fin code copy from MIOpen, and instead it is tracked as a submodule. This change would remove the diffs we see in MIOpen when we change Fin. MIOpen changes here are for how the Jenkinsfile interacts with and builds the Fin repo.

def no_cache = conf.get("no_cache", false)
def dockerArgs = "--build-arg BUILDKIT_INLINE_CACHE=1 --build-arg PREFIX=${prefixpath} --build-arg GPU_ARCH='${gpu_arch}' --build-arg MIOTENSILE_VER='${miotensile_version}' --build-arg USE_TARGETID='${target_id}' --build-arg USE_MLIR='${mlir_build}' --build-arg USE_FIN='${build_fin}' "
def dockerArgs = "--build-arg BUILDKIT_INLINE_CACHE=1 --build-arg PREFIX=${prefixpath} --build-arg GPU_ARCH='${gpu_arch}' --build-arg MIOTENSILE_VER='${miotensile_version}' --build-arg USE_TARGETID='${target_id}' --build-arg USE_MLIR='${mlir_build}' "
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove USE_FIN from Dockerfile.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cderb Let's remove unused code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@atamazov should USE_FIN be removed from everywhere in the repo? I may need to update dockerfile too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@junliume I do not know. But removal of USE_FIN='${build_fin}' at this line in Jenkinsfile makes USE_FIN unused in the Dockerfile, so it should be removed.

@cderb cderb deleted the jd/update_fin branch February 28, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants