Skip to content

Conversation

@MohdAatifSiddi
Copy link
Contributor

@MohdAatifSiddi MohdAatifSiddi commented Jul 12, 2025

Description

Replace TODO comment with actual implementation
Add os.makedirs() call to ensure log directory exists before file operations
Prevents FileNotFounderror when accessing solver logs

Issue

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

   - Replace TODO comment with actual implementation
   - Add os.makedirs() call to ensure log directory exists before file operations
   - Prevents FileNotFoundError when accessing solver logs
@MohdAatifSiddi MohdAatifSiddi requested a review from a team as a code owner July 12, 2025 09:14
@MohdAatifSiddi MohdAatifSiddi requested a review from Iroy30 July 12, 2025 09:14
@copy-pr-bot
Copy link

copy-pr-bot bot commented Jul 12, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

…_client_solver_settings.py

    and corrected "concelation" to "cancellation" in job_queue.py
@rgsl888prabhu
Copy link
Collaborator

Thank you @MohdAatifSiddi for the PR, PR looks good. I will enable CI and if everything goes smooth we can merge the PR.

@MohdAatifSiddi Please feel free to create issues and PRs to make cuOpt better and serve your needs.

@rgsl888prabhu rgsl888prabhu added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Jul 16, 2025
@rgsl888prabhu rgsl888prabhu added this to the 25.08 milestone Jul 16, 2025
@rgsl888prabhu
Copy link
Collaborator

/ok to test 095dcd2

@tmckayus
Copy link
Contributor

This change does not actually result in the directory being created, because there is already a check when the result dir location is set as to whether the directory exists and is writable by the process (settings.py, set_result_dir())

The intention is to fail immediately on startup. The results directory is used as an alternate means of returning results, instead of sending over http. It also doubles as the log directory (perhaps it shouldn't and a log directory should be separate). Since results are really important, a user ought to know where they are and ought to be sure they have access to the directory before running problems. Hence, we do not create it for them.

A log directory, though -- if we separate it out I could see that.
My suggestion is close this PR and create an issue to separate the log dir from the results dir.

$ python -m cuopt_server.cuopt_service -r /bill
:128: RuntimeWarning: 'cuopt_server.cuopt_service' found in sys.modules after import of package 'cuopt_server', but prior to execution of 'cuopt_server.cuopt_service'; this may result in unpredictable behaviour
2025-07-25 14:52:20.419 INFO cuopt server version 25.08.00
2025-07-25 14:52:20.421 DEBUG cuopt_server/utils/request_filter.py:set_tier Tier is managed_default
2025-07-25 14:52:20.423 INFO cuopt_server/utils/settings.py:set_result_dir Result directory is '/bill', maxresult = 250, mode = 644
Traceback (most recent call last):
File "", line 198, in _run_module_as_main
File "", line 88, in _run_code
File "/home/tmckay/miniforge3/envs/cuopt_dev/lib/python3.12/site-packages/cuopt_server/cuopt_service.py", line 358, in
settings.set_result_dir(args.resultdir, args.maxresult, args.mode)
File "/home/tmckay/miniforge3/envs/cuopt_dev/lib/python3.12/site-packages/cuopt_server/utils/settings.py", line 65, in set_result_dir
raise ValueError(f"Result directory '{dir}' " "does not exist!")
ValueError: Result directory '/bill' does not exist!

@tmckayus
Copy link
Contributor

In fact, even in the default case this crashes the log callback mechanism in the webserver because os.makedirs("") is an error. The default value of the result dir is "", ie an empty path. Trace with an added exception handler in cuopt/log/{id} endpoint.

2025-07-25 15:30:07.231 DEBUG job_result returning msgpack
2025-07-25 15:30:07.232 INFO 127.0.0.1:38794 - "GET /cuopt/solution/755df170-c31a-44c7-ac09-b8dcca1e84df HTTP/1.1" 200
Traceback (most recent call last):
File "/home/tmckay/miniforge3/envs/cuopt_dev/lib/python3.12/site-packages/cuopt_server/webserver.py", line 352, in getsolverlogs
os.makedirs(log_dir, exist_ok=True)
File "", line 225, in makedirs
FileNotFoundError: [Errno 2] No such file or directory: ''

@tmckayus
Copy link
Contributor

The typo fixes are good, we'll keep those and file an issue to separate log dir from result dir.

@tmckayus tmckayus changed the title Fix: Create log directory if it doesn't exist in webserver Fix: various typos in comments and strings, note on result dir Jul 25, 2025
@tmckayus
Copy link
Contributor

/ok to test 4aec192

@rgsl888prabhu
Copy link
Collaborator

/ok to test 3810819

@tmckayus
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 2a60e42 into NVIDIA:branch-25.08 Jul 28, 2025
211 of 215 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants