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

Get files changed in PR #11825

Merged
merged 47 commits into from
Nov 24, 2023
Merged

Get files changed in PR #11825

merged 47 commits into from
Nov 24, 2023

Conversation

philbucher
Copy link
Member

@philbucher philbucher commented Nov 19, 2023

This PR is the first step towards improving the CI by not always compiling everything. E.g. not always all apps need to be compiled, or not Cpp tests need to be compiled if only python files were changes. Stuff like this, ofc TBD once we actually want to introduce it

In a nutshell, this PR uses the github cli to get obtain the files modified in the PR. For now I am only printing them to observe it for a while. Eventually we can use them to refine the CI, see above

@philbucher philbucher marked this pull request as ready for review November 20, 2023 07:44
@philbucher philbucher requested a review from a team as a code owner November 20, 2023 07:44
@roigcarlo
Copy link
Member

Were is the list printed? the CI only shows a variable :S

@matekelemen
Copy link
Contributor

@philbucher can you plz give a quick rundown on what this does?

@philbucher philbucher changed the title WIP show files changed in PR Get files changed in PR Nov 22, 2023
@philbucher philbucher added the Continuous Integration related to Travis, Appveyor, ... label Nov 22, 2023
@philbucher
Copy link
Member Author

Sorry I marked too early as ready. I updated the description of the PR

@roigcarlo
Copy link
Member

roigcarlo commented Nov 23, 2023

Out of curiosity, how are you planning to get the list of what needs to be run based on the modified files? I remember that I tried that once and I just gave up.

I specially had problems with multilevel dependencies. for example:

I modify process pA

There is a script in the application aX that does not depend on pA that uses process pB. process pB uses process pC from app aY and this process pC (which is pure python) callas the pA that I've modified.

In this case the CI would need to compile:

  • Kernel (that's easy)
  • App aY ( and maybe others) that directly make use of pAat python level. Sort of easy as well parsing all the python interface
  • App aX because one of its python scripts calls the python script form aY. Harder, as I have to parse all the python interface and have the python interface of all other apps parsed as well to know the potential call graph

@roigcarlo
Copy link
Member

Also, consider that this: https://github.com/KratosMultiphysics/Kratos/tree/core/new-compile-pipeline will be merged soon™

@philbucher
Copy link
Member Author

Also, consider that this: https://github.com/KratosMultiphysics/Kratos/tree/core/new-compile-pipeline will be merged soon™

I know, this will not conflict. In any case, I would adapt the developments to it if necessary

@philbucher
Copy link
Member Author

Regarding the dependencies: I had similar thoughts.
In the end I want to add a small file in each app, where the dependencies for the CI are specified :) Then the devs can decide themselves what needs to be run.
E.g. for the StructuralMechanicsApp: LinearSolversApp, ConstitutiveLawsApp
For CoSim: StructuralMechanics (and thus also the LinearSolversApp and the ConstLawApp), FluidDynamics, ...

Given that a core change (which usually also happens daily) compiles everything, same as the nightly build I would say we are on the safe side. But I would discuss the details in the future

@philbucher philbucher merged commit c133a6e into master Nov 24, 2023
17 checks passed
@philbucher philbucher deleted the ci/changed-files branch November 24, 2023 13:42
@philbucher philbucher mentioned this pull request Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Continuous Integration related to Travis, Appveyor, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants