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

Replace old os.path calls by pathlib correspondents - part 1 #9708

Closed
wants to merge 8 commits into from

Conversation

ErickGiffoni
Copy link
Contributor

@ErickGiffoni ErickGiffoni commented Sep 23, 2021

This PR partially addresses #3153.

Me, @jps12 and @caue96 are continuing the work that was being done previously in #7383 and #7118.

While this PR makes changes to some directories under rasa/, we will be doing 2 or 3 other separated PRs,
as suggested by @wochinge, so the changes can be reviewed more easily.

Proposed changes:

  • in general: replace os.path calls with each pathlib.Path correspondent.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

caue96 and others added 6 commits September 22, 2021 19:35
Co-authored-by: Erick Giffoni <[email protected]>
Co-authored-by: João Pedro <[email protected]>
Co-authored-by: Erick Giffoni <[email protected]>
Co-authored-by: João Pedro <[email protected]>
Co-authored-by: Erick Giffoni <[email protected]>
Co-authored-by: João Carvalho <[email protected]>
Co-authored-by: João Carvalho <[email protected]>
Co-authored-by: Caue Matheus <[email protected]>
@ErickGiffoni ErickGiffoni requested a review from a team as a code owner September 23, 2021 18:23
@ErickGiffoni ErickGiffoni requested review from a team, ancalita and kedz and removed request for a team September 23, 2021 18:23
@CLAassistant
Copy link

CLAassistant commented Sep 23, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

@ErickGiffoni Thanks for your contribution!

There is unfortunately one issue: We are currently doing a big revamp of the model architecture which will cause a lot of changes in the training / testing and prediction (server, Agent) modules. We aim to wrap this up by the end of next but until then I'd like to put this on hold as this will cause quite a lot of merge conflicts and I want to avoid making the quite complex integration even more messy. Would that be okay for you?

@ErickGiffoni
Copy link
Contributor Author

@ErickGiffoni Thanks for your contribution!

There is unfortunately one issue: We are currently doing a big revamp of the model architecture which will cause a lot of changes in the training / testing and prediction (server, Agent) modules. We aim to wrap this up by the end of next but until then I'd like to put this on hold as this will cause quite a lot of merge conflicts and I want to avoid making the quite complex integration even more messy. Would that be okay for you?

Ok, me and my colleagues understand !
Do you think we better make the other 3 PRs mentioned targeting branch 2.8.x,
or should we wait... or maybe make them as drafts ?

@wochinge
Copy link
Contributor

wochinge commented Oct 4, 2021

Thanks for understanding this! I think 2.8.x would eventually cause the same merge conflicts as we'd have to merge that to main eventually. Drafts would be the better solution (however, they have the risk that you'd do some duplicate work). We are basically touching all modules related to training and prediction (Agent, Processor) - you can also see our current state here. Anything outside of this should be fine to touch

@wochinge
Copy link
Contributor

@ErickGiffoni We've just merged all of our architecture changes 🎉 You can now safely work on using Path in any files in the codebase without having to fear bigger refactorings. Thanks for holding off with this change 🙌🏻

@ancalita
Copy link
Member

@ErickGiffoni You can now safely work on using Path in any files in the codebase without having to fear bigger refactorings.

Actually, there is another big change on the horizon, could you please hold off with any changes in these modules, if you find any 🙏🏼 :

  • rasa/core/actions/action.py
  • rasa/core/actions/forms.py
  • rasa/core/processor.py
  • rasa/cli/data.py
  • rasa/cli/arguments/data.py
  • rasa/shared/core/domain.py
  • rasa/shared/core/slots.py

@ErickGiffoni
Copy link
Contributor Author

@wochinge and @ancalita ok, sure, thanks !
We will get back to it soon and hold off the specific modules.

Merge branch 'main' of git://github.com/RasaHQ/rasa into use-pathlib
@ErickGiffoni
Copy link
Contributor Author

Conflicting files that required changes (in relation to the use of os.path)

  • rasa/model_testing.py
  • rasa/model.py
  • rasa/core/train.py
  • rasa/core/test.py
  • rasa/core/agent.py

@ErickGiffoni
Copy link
Contributor Author

@juliolitwin, @jps12, @caue96, @andrelucasf hey guys, let's take look at this PR
and maybe prioritise it. Tests are failing.

@ErickGiffoni
Copy link
Contributor Author

Also, here is a good reference for this PR:
https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module

@ancalita
Copy link
Member

@wochinge and @ancalita ok, sure, thanks ! We will get back to it soon and hold off the specific modules.

@ErickGiffoni You have green light to continue now in any module, the big change I mentioned was merged yesterday 🎉

@ErickGiffoni
Copy link
Contributor Author

Hey guys, I'd like to inform in advance that from Nov 10 to Dec 13 (at least)
I'l be working on another project at Gloria Institute in a partnership with Microsoft.
Because of that I won't be working on this PR for that period of time. I apologise, but
when I get back I can let you know if I'll still be able to finish this here !
Thanks for understanding :)

@SamuelNoB SamuelNoB deleted the use-pathlib branch January 27, 2022 22:00
@ancalita
Copy link
Member

ancalita commented Apr 7, 2022

Hi @ErickGiffoni hope all is well! Double-checking if you're still interested in completing this PR? If so, since main branch has diverged quite significantly since Nov-21, I'd advise opening a new PR. Otherwise, if not interested any longer, please close this PR.

@ErickGiffoni
Copy link
Contributor Author

Hi @ErickGiffoni hope all is well! Double-checking if you're still interested in completing this PR? If so, since main branch has diverged quite significantly since Nov-21, I'd advise opening a new PR. Otherwise, if not interested any longer, please close this PR.

Hey @ancalita tks for checking in on me, hope all is well with you too.
I'll close this PR then. For now I'm very very busy and I can't continue
working on this. But hopefully another time I can contribute more !

=)

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.

5 participants