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

codegen output and GIT #416

Closed
lagacep-ans opened this issue May 13, 2022 · 11 comments · Fixed by #420 or #421
Closed

codegen output and GIT #416

lagacep-ans opened this issue May 13, 2022 · 11 comments · Fixed by #420 or #421
Assignees

Comments

@lagacep-ans
Copy link
Contributor

If you run the codegen tools, for an up to date version of the APIs, you'll end up with many GIT "modified files" created by that operation.

Should we tackle this by having two locations for the APIs?

  • The built-in , commited in pyfluent
  • The local version, used if its there.
@h-krishnan
Copy link
Contributor

As the codegen files (at least the settings files) are not really required for runs, could we generate them as part of the packaging and not really commit them into git?

@dnwillia-work
Copy link
Collaborator

@lagacep-ans Patrick, are you seeing differences even if you pip install -r requirements_codegen.txt into a clean virtual environment. I ran into this issue in the past due to a difference in grpcio-tools.

@dnwillia-work
Copy link
Collaborator

As the codegen files (at least the settings files) are not really required for runs, could we generate them as part of the packaging and not really commit them into git?

Yeah, we could do that, though it takes some time to build the different modules, and we would have to generate the documentation from them right.

@lagacep-ans
Copy link
Contributor Author

My original question was related to the scenario where you are using a different version of fluent, and I was assuming the difference came from that.
But yes, tried it with v222... and the large amount of difference is the same

Tried that in a plain venv:
image

and the differences are ... different formatting only
image

that would be related to the requirement versions? or something else in the environment applying precommit formatting.

@h-krishnan
Copy link
Contributor

I think the code generator was updated but possibly the generated files were never merged. @V-Dandekar Do you know anything about this?

@lagacep-ans
Copy link
Contributor Author

or... some flake/black/isort... was executed on these files. but these are part of the exception rules for the precommit hooks

@V-Dandekar
Copy link
Contributor

V-Dandekar commented May 17, 2022

@lagacep-ans @h-krishnan, With codegen the files are generated but we need to run "make" to format them before we commit (Which shouldn't be any extra effort as it is recommended for any normal commit too.) Please run make and then check the diff. If we decide to generate files on server then we will have to ensure that make is run after that. Or modify the script that will comply with formatting rules. But I think running make is easier since it is difficult to code the formatting and we may always end up in a corner case

@lagacep-ans
Copy link
Contributor Author

windows: make is not an option in all shells, but yes running the pre-commit command line manually does some job on the files
image

heere, still lot of differences after doing that. mostly the order of import statements, and some whitespace.
this is a bit dependent of the workflow used, and the environment of the developer.
and we end up with hundreds different files, which cannot be added to .gitignore.

anyway, i got workarounds, but this process is to improve IMHO

@dnwillia-work
Copy link
Collaborator

dnwillia-work commented May 17, 2022

In the makefile the commands are there to regenerate it properly, so you just type this:

make api-codegen

and that should give the final output. You should not need to run pre-commit or anything I think.

You are right about windows and make. For the documentation at least I made a make.bat, but we could do something similar for the main Makefile. You would need to update things 2x but it's kind of the way it is unless we use a consistent make system on both platforms.

@mkundu1
Copy link
Contributor

mkundu1 commented May 17, 2022

Note that, the make api-codegen target is going through some fixes in PR 420.

@mkundu1 mkundu1 linked a pull request May 18, 2022 that will close this issue
@mkundu1
Copy link
Contributor

mkundu1 commented May 18, 2022

Removed the generated files and generating those files under packaging in PR 420.

@mkundu1 mkundu1 self-assigned this May 18, 2022
@mkundu1 mkundu1 linked a pull request May 18, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants