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

Update output file location #37

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Update output file location #37

merged 3 commits into from
Nov 27, 2023

Conversation

nreinicke
Copy link
Collaborator

@nreinicke nreinicke commented Nov 21, 2023

Takes a pass at #32 by adding a new optional argument output_file_override to the from_config_file method on the python compass application. This would allow the user to load the application as:

app = CompassApp.from_config_file("my_config.toml:, output_file_override="my_output.json")

I also played around with changing the output file after the application has been loaded (see the rust CompassApp.with_output_directory() method) but we can roll that back if seems too weird.

Going through this it still feels a bit awkward but I had a hard time finding an elegant way to give a custom config parameter outside of the context of the config file. I'm open to alternatives if you have any ideas.

@robfitzgerald
Copy link
Collaborator

I'm open to alternatives if you have any ideas

this does seem to go pretty deep to deliver this feature. a shallower solution could work mostly in the TOML.

if the user sets the output_file_override argument, it would first deserialize the TOML from file in python, modify it's outfile location directly, and then pass the modified TOML blob (maybe as a string) to CompassApp.

to use that on the rust-side, we would need yet another CompassApp.try_from method, but this time, something like a TryFrom<&str> that expects it needs to parse the string into a Config. it would follow a similar logic to CompassApp's TryFrom<&Path> but add a step to deserialize the Config. but then we don't need to make any other changes on the rust code.

@nreinicke
Copy link
Collaborator Author

if the user sets the output_file_override argument, it would first deserialize the TOML from file in python, modify it's outfile location directly, and then pass the modified TOML blob (maybe as a string) to CompassApp.

to use that on the rust-side, we would need yet another CompassApp.try_from method, but this time, something like a TryFrom<&str> that expects it needs to parse the string into a Config. it would follow a similar logic to CompassApp's TryFrom<&Path> but add a step to deserialize the Config. but then we don't need to make any other changes on the rust code.

Yeah that is a much cleaner and less intrusive solution, let me run with that, thanks for the quick suggestion!

@nreinicke
Copy link
Collaborator Author

Okay, I've updated to come at this from the python side and just do a manipulation on the config and pass it to rust as a string. It still doesn't feel perfect but it's much less intrusive an I think it gets us what we want in terms of functionality

Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

i tried to test this using the notebook example:

$ git checkout ndr/update-output-dir
$ git status
On branch ndr/update-output-dir
Your branch is up to date with 'origin/ndr/update-output-dir'.
$ conda activate routee-rust
$ pip install -e .

then, in the notebook, running in routee-rust, i added the output file argument:

app = CompassApp.from_config_file("golden_co/osm_default_energy.toml", output_file="foo.json")  

that should have created an output file, but, even after calling app.run there is no such file.

two thoughts

  • i think it fails because there is no out file output plugin in the TOML. maybe the logic should inject (append to the end of the list) the output plugin if it's missing?
  • or maybe i'm doing something wrong, can you confirm/let me know if this does work for you?

@nreinicke
Copy link
Collaborator Author

Yeah this is definitely due to the fact that there is no to_disk plugin specified in the osm_default_energy.toml file. I had initially put a warning in but I think the expected behavior would be to add that plugin into the config if a user passes in that optional argument. Thanks for testing and catching this.

@nreinicke
Copy link
Collaborator Author

@robfitzgerald - this has been updated to inject a to_disk plugin if the output directory is specified and one does not already exist

Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

works for me!

@nreinicke nreinicke merged commit fdf8956 into main Nov 27, 2023
5 checks passed
@nreinicke nreinicke deleted the ndr/update-output-dir branch November 27, 2023 16:55
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