https://lakefs.io/ logo
Join Slack
Powered by
Hi Team I am working on this issue <https://github...
# dev
v
Hi Team I am working on this issue https://github.com/treeverse/lakeFS/issues/2344 As far as I know I have to change the the README under the python package https://github.com/treeverse/lakeFS/tree/master/clients/python . Not sure why the changes doesn't reflect in pypy of lakefs already. Or if it is not how it was supposed to be done please guide me where to add the changes?
g
Hi @Vaibhav Kumar, Thanks for contributing sunglasses lakefs You are correct, It seems like the README is currently not being used for some reason, part of this issue is understanding whats not working there. I will give it a look tomorrow and help you out with the next steps.
v
So ideally the below read me should have displayed on pypy as well right? https://github.com/treeverse/lakeFS/blob/master/clients/python/README.md
g
Hi @Vaibhav Kumar, The description you see in pypi is taken from the 
long_description
field in the file https://github.com/treeverse/lakeFS/blob/master/clients/python/setup.py You should edit the long description, so it will contain the information requested in the issue. Just notice this file is generated using open-api ( you can’t just edit it) This data is generated in the Makefile (
client-python: api/swagger.yml  ## Generate SDK for Python client
)
v
I need to edit this README and there will be couples of code line changes in Setup.py . Please confirm if this approach is ok with you?
g
Unfortunately that won’t be enough, you would need to edit the makefile, in a way that when running 
make client-python
the 
long_description
in 
Setup.py
will include a link to the 
pydocs
.
v
Whatever we see on the pypy site is from the text present in the 
long_description
and so just changing the setup and recompiling should work
What I have read from internet is you just need to edit setup.py, recompile your makefile and that should be enough. I didn't see it anywhere to edit the makefile. If you have any doc to refer can you please share?
g
The python client is automatically generated every time we change the api If you just change the 
setup.py
file, the next time the python client will be generated your changes will be overwritten. You could mimic it running the following steps: 1, make a change in 
setup.py
2. Run 
make client-python
from the root of the project 3. go back to 
setup.py
- your changes where removed your change should still exist after running 
make client-python
The python client is built in the Makefile, using the 
openapi-generator
you could read about it more here
v
where are we uploading the python distribution package to pypi , I couldn’t find it in 
Makefile
or 
swagger.yml
@Guy Hardonag
g
We don't, In the makefile we run make client-python That command generates the python client code And the setup.py file
v
ok, then how do you put the python lakefs package to pypi repository ? I am a bit confused
g
If you would like to see how that happens you could look at https://github.com/treeverse/lakeFS/blob/master/.github/workflows/python-api-client.yaml This workflow builds (using the makefile) and publishes (to pypi ) the python client.
v
I have tried putting a lakefs readme on a sample package on pypi test and it is working fine. I Have uploaded my package using twine. As you said earlier`make client-python` has replaced my code, now I am lost what and where I am supposed to change to avoid this overwritten behaviour . Kindly help.
g
Looks good 👍 If you’d like, we could meet and talk about it on Sunday or Monday.
v
Sure we can, Sunday works for me
Please let suggest sometime
👍 1
g
Moving to a private chat
v
Hi @Guy Hardonag I tried my best to find the README param in open API but I didn’t find any such thing. Could you please help?
g
Hi @Vaibhav Kumar , thanks! looks like we will need to add a script that will edit the 
setup.py
file after the python client is generated. I will look into it today and get back to you by the end of the day
👍 1
v
So whatever changes I did in setup.py in the forked repo are those needed as of now?
g
Yes, we will need to write a script that injects your changes
Hi @Vaibhav Kumar , I’m sorry, didn't get to it today I’m going on vacation tomorrow. @Itai David will help you out.
v
Sure, @Guy Hardonag enjoy your vacation
🙏 1
@Itai David I hope you are aware about the issue, let me know if you need any details
i
Hi @Vaibhav Kumar @Guy Hardonag already filled me in on the details.
v
Ok, so how shall we proceed then?
i
As far as I understood, I will need to update our build process - probably add a script - to add your work to 
setup.py
, after the client is generated. Did I get this correctly?
v
Yes correct
i
I will have to dig in a bit, in order to find out how to do that, and get back to you with that. OK?
v
Ok
🙂 1
i
Hi @Vaibhav Kumar I haven't completed this one yet. I will put some more effort tomorrow (my tomorrow 🙂 ) and get back to you with an update Thank you
v
Works for me, once my changes are deployed I will pick up the next issue ☺️
👍 1
@Itai David
Hi @Itai David Any updates?
i
Hi @Vaibhav Kumar. I'm still trying to figure this one out. There seem to be a straight-forward way to set this update, using ppenapi generator arguments, but for some reason it does not work and openapi generator ignores it. I'm trying to figure out if this is a bug that can be solved easily (e.g. updating openapi generator etc.) but so far with no success Will update you with any progress
v
What all arguments you think should work?
i
I was looking at the 
--additional-properties
flag. Looking at the code it seems it should allow an override with 
appDescription
key, but looks like it is ignored
v
Yes , @Guy Hardonag also tried it but didn't work. So he said about making the script
i
Yes. The script he was referring to is a bit of a hack. I'm trying to see if there's a more straight forward way to achieve the same result. Currently looking on how to use user-defined templates
v
Ok , please keep me posted for any updates
Meanwhile what else shall I pick considering I know python and java
i
Sorry for the delay - I somehow forgot to hit the send button... We have this one: https://github.com/treeverse/lakeFS/issues/2801. It involves another client - the hadoopFS client and will require some 
lakeFS
work (bringing a server up, configuring the client etc.) - we should have all the required info documented, with most of it as a step-by-step guides). It will probably need some 
spark
work too - in case you are up to it
Hi @Vaibhav Kumar. Great update: I have https://github.com/treeverse/lakeFS/pull/4230 in review. It adds a user-defined template to generate 
setup.py
. It is currently identical to the one used by OpenAPI Generator, but you can edit the 
long_description
string to fit your needs. Feel free to branch-out from my branch https://github.com/treeverse/lakeFS/tree/2344-pyhton-client-project-description, or to merge it to yours. Good luck
v
@Itai David Ok, So now I can pull from https://github.com/treeverse/lakeFS/tree/2344-pyhton-client-project-description and just add my changes and then give you the fork right?
i
Hi @Vaibhav Kumar - Once you are done with your work you can open a PR
v
@Itai David if I am applying my changes and and then using 
make client-python
is replacing my changes. I have pulled your branch to apply these changes on.
@Itai David https://github.com/treeverse/lakeFS/pull/4241 Please take a look
👀 1
I have raised the PR
i
Did you try updating the 
setup.mustache
template file I added. The generator creates 
setup.py
based on that template. The one I submitted is the default one. I think that be adding your changes to 
setup.mustache
will have them reflected in 
setup.py
. Can you give it a go?
v
@Itai David I have put my changes to 
setup.mustache
but if I clone from your branch I am not able to raise a PR. I get permission denied, I am not able to create multiple forks as well.
i
Hi @Vaibhav Kumar I'm not really sure why is that. Can you try and merge my branch into yours, or rebase your branch from mine. I think it will add my commit to your branch and PR, and will let you merge it all together
v
Hi @Itai David I have merged your branch to mine and raised the below PR https://github.com/treeverse/lakeFS/pull/4260
Let me know if you have any queries.
i
Hi @Vaibhav Kumar. Thank you for that. Let me take a look at it 🙂
Hi Again @Vaibhav Kumar, and thanks again for this contribution. I reviewed the code and it looks great. However, I'm not a part of the team usually handling the clients code, so I would like it to be reviewed by that team. As today is a holiday in Israel, they will probably only be available to review it tomorrow. I will keep you posted
v
That is fine @Itai David
I was going through the checks and seems like one check 
Test / Run Linters and Checkers (pull_request)
has failed and rest has passed, is that fine?
i
Hi @Vaibhav Kumar - that's something that needs to be addressed. It verifies that the newly generated client matches the submitted code. I think that generating the client based on your recent version, and adding the generated code to the PR, should fix that. Can you please give it a try?
v
I ran 
make client-python
again which auto generated code to setup.py with my changes which I had put in 
setup.mustache
. Is that what you were asking for? @Itai David
i
Yes please. We need the committed 
setup.py
to match the one that will be generated by the build Thank you @Vaibhav Kumar
v
Done updated my PR
👀 1
i
Looks great. I will wait for the checks to complete and ping the relevant team to prioritize the review
I pinged them. Will keep you posted 🙂
12 Views
Open in Slack
PreviousNext
Powered by