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 tmpdir usage with tmp_path in our tests #16188

Closed
carmocca opened this issue Dec 23, 2022 · 21 comments · May be fixed by #18382
Closed

Replace tmpdir usage with tmp_path in our tests #16188

carmocca opened this issue Dec 23, 2022 · 21 comments · May be fixed by #18382
Assignees
Labels
good first issue Good for newcomers tests
Milestone

Comments

@carmocca
Copy link
Contributor

carmocca commented Dec 23, 2022

Outline & Motivation

tmpdir is considered legacy:

The tmpdir and tmpdir_factory fixtures are similar to tmp_path and tmp_path_factory, but use/return legacy py.path.local objects rather than standard pathlib.Path objects.

(from https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html#the-tmpdir-and-tmpdir-factory-fixtures)

Pitch

Run pytest with pytest -p no:legacypath as the link above suggests and replace all occurrences.

This can be added in https://github.com/Lightning-AI/lightning/blob/94e6d52b7e2f2a9ffc21f7e11e087808666fe710/setup.cfg#L28-L32

 addopts =
     --strict-markers
     --doctest-modules
     --color=yes
     --disable-pytest-warnings
     --ignore=legacy/checkpoints
+    -p no:legacypath

Replace all tmpdir usages

Additional context

No response

cc @Borda

@carmocca carmocca added good first issue Good for newcomers tests labels Dec 23, 2022
@carmocca carmocca added this to the future milestone Dec 23, 2022
@Saumya-ranjan Saumya-ranjan removed their assignment Dec 25, 2022
@yhl48
Copy link
Contributor

yhl48 commented Jan 4, 2023

Happy to contribute to this issue if no one has been assigned to it!

@subham73
Copy link

Hello, @yhl48 are you still working on this? I'm planning to work on this, would love to have your input on this. Thanks

@Aditi840
Copy link

@carmocca, in which file tmpdir with tmp_path should be replaced

@Borda
Copy link
Member

Borda commented Aug 23, 2023

@carmocca, in which file tmpdir with tmp_path should be replaced

in all files :)

@Aditi840
Copy link

All files of lightning-AI, can I work on it.

1 similar comment
@Aditi840
Copy link

All files of lightning-AI, can I work on it.

@Borda
Copy link
Member

Borda commented Aug 24, 2023

All files of lightning-AI, can I work on it.

Let's split is per topic:

@Aditi840
Copy link

May I do the above 4 files.

@Aditi840
Copy link

@Borda Got it, replacements have to be done on tests file, I'm starting working on tests files.

@Aditi840
Copy link

@Borda will you please check my pull request and the changes I have done.

@Borda
Copy link
Member

Borda commented Aug 24, 2023

@Borda will you please check my pull request and the changes I have done.

I think it would be easier/faster if you create a PR per check item here: #16188 (comment)

@Aditi840
Copy link

@Borda , what we should do files, give me a idea

@Borda
Copy link
Member

Borda commented Aug 26, 2023

@Borda , what we should do files, give me a idea

Let's create another PR and perform replacement just for one sub folder as listed in comment/check-list above...

@Aditi840
Copy link

@Borda what replacement do we need to perform and on which sub-folder

@Borda
Copy link
Member

Borda commented Aug 26, 2023

@Borda what replacement do we need to perform and on which sub-folder

Could you please ping me on slack, would be easier to explain =)

@Aditi840
Copy link

@Borda , yaa sure tell me your slack Id

@Borda
Copy link
Member

Borda commented Sep 20, 2023

@Borda , yaa sure tell me your slack Id

You can find me as Jirka :)
I made a sample partial fix, so based on partitions in #16188 (comment), I just chose a topic/folder and tried to apply changes...

  1. chose one folder and create a draft PR (which can be almost empty) that we can link the issues
  2. take it file by file and perform replacement
  3. I would suggest each file change shall be one commit in the PR so it is easier to trace what changes are not compatible

@Aditi840
Copy link

@Borda ok, will do that.

@Borda
Copy link
Member

Borda commented Sep 21, 2023

@Borda ok, will do that.

see sample #18604

@fnhirwa
Copy link
Contributor

fnhirwa commented Mar 5, 2024

@Borda is this still open for contribution, I would love to work on it 😊

@Borda
Copy link
Member

Borda commented Mar 5, 2024

@Borda is this still open for contribution, I would love to work on it 😊

great, I can still see many places where this would need to be replaced :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants