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

Fix-long-filenames for docker arguments #1305

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

jahaniam
Copy link
Contributor

Hi Neptune team,
We're doing a quick prototype using your library and during the prototyping, we have faced the following easy to fix issue.

If you use a container and pass very long arguments to the container (in our case the orchestrator does it and we have no control over it), the input argument is captured within sys.argv and later in the code it will result in exception error OSError: [Errno 36] File name too long

In pathlib documents it also mentions that path.is_file() can throw errors like permissions ( in our case filename being too long).

Solution:
you should use error handling. This won't usually happen in the normal execution and only happens in container-based orchestrators as one of the argv could be very long.

We're currently blocked by this issue. We'd appreciate it if you can generate a quick bugfix release from this PR. This allows us to use your library in our prototype going forward.

PS.

  1. Is there documentation on how I can locally build this package?
  2. I wasn't sure about the new versioning so I didn't change the CHANGELOG. Feel free to do it as you see fit.

Thanks,
Ali

Testcase:

long_str = str(10**255)
path = Path(long_str)
print(path.is_file()) -> Throws an Error filename too long. 

@Raalsky
Copy link
Contributor

Raalsky commented Mar 21, 2023

Hey @jahaniam, thanks a lot for submitting this PR 😉. I much appreciate that!

I wasn't sure about the new versioning so I didn't change the CHANGELOG. Feel free to do it as you see fit.

Sure, I'll update a CHANGELOG in a moment.

Is there documentation on how I can locally build this package?

It should be as easy as pip install -e ".[dev]". We're going to prepare a contributing guide, but we don't have something publicly available.

@Raalsky
Copy link
Contributor

Raalsky commented Mar 21, 2023

I've updated CHANGELOG and added a minor unit test. I hope that you don't mind.

@Raalsky Raalsky merged commit 876a886 into neptune-ai:master Mar 21, 2023
@jahaniam
Copy link
Contributor Author

@Raalsky Thanks for looking into it!

@Raalsky
Copy link
Contributor

Raalsky commented Mar 22, 2023

Released in 1.1.1. Thanks a lot 😉

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.

2 participants