Skip to content

Conversation

@gregjacobus
Copy link
Collaborator

Better default config values

Description

A number of the default configuration parameters are pretty useless as is. Setting them to reasonable defaults will make the process of installing FIREWHEEL much simpler.

Related Issue

N/A

Type of Change

Please select the type of change your pull request introduces:

  • Bugfix
  • New feature
  • Documentation update
  • Other (please describe):

Checklist

  • This PR conforms to the process detailed in the Contributing Guide.
  • I have included no proprietary/sensitive information in my code.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have tested my code.

Additional Notes

N/A

@github-actions github-actions bot added the feature New feature or request label Dec 23, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 23, 2025
@gregjacobus gregjacobus marked this pull request as ready for review December 23, 2025 14:26
Copy link
Member

@mitchnegus mitchnegus left a comment

Choose a reason for hiding this comment

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

These all seem reasonable to me—localhost seems well defined, and I think most any Unix system will have a /tmp directory, so I have no objections.

I'll defer to Steven on whether there might be reasons to not assume names based on historical context.

@mitchnegus mitchnegus changed the title Feat: Better default config values feat: Better default config values Dec 23, 2025
Copy link
Member

@mitchnegus mitchnegus left a comment

Choose a reason for hiding this comment

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

These are minor suggestions for the wording. I think they are okay as is, but the wording "left" implies that they default to the empty string (which will no longer be true).

+-------------+----------+------------------+------------------------------------------------------------------------------------------------------------------------------------------------------+
|``port`` |int |``50051`` |The port number for FIREWHEEL's gRPC service. |
+-------------+----------+------------------+------------------------------------------------------------------------------------------------------------------------------------------------------+
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If left empty, the ``system.default_output_dir`` value will be used. |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If left empty, the ``system.default_output_dir`` value will be used. |
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If set as an empty string, the ``system.default_output_dir`` value will be used as the fallback. |

|.. _config-root_log_dir: | | | |
| | | | |
|``root_dir`` |string |``""`` |The path to the log files. If left empty, the ``system.default_output_dir`` value will be used. |
|``root_dir`` |string |``/tmp/firewheel`` |The path to the log files. If left empty, the ``system.default_output_dir`` value will be used. |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|``root_dir`` |string |``/tmp/firewheel`` |The path to the log files. If left empty, the ``system.default_output_dir`` value will be used. |
|``root_dir`` |string |``/tmp/firewheel`` |The path to the log files. If set as an empty string, the ``system.default_output_dir`` value will be used as the fallback. |

+=============+==========+==================+===================================================================================================+
|``cache_dir``|string |``fw_cli`` |The folder name of the CLI Helper cache. |
+-------------+----------+------------------+---------------------------------------------------------------------------------------------------+
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If left empty, the ``system.default_output_dir`` value will be used.|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If left empty, the ``system.default_output_dir`` value will be used.|
|``root_dir`` |string |``/tmp/firewheel``|The path to the ``cache_dir``. If set as an empty string, the ``system.default_output_dir`` value will be used as the fallback. |

@gregjacobus
Copy link
Collaborator Author

@mitchnegus I do agree with all the wording suggestions you made. It turns out, however, that it falling back to system.default_output_dir is simply not true, which is why having these not be set is a problem to begin with. I think this PR should change to fixing that bug instead. The only default argument then that would need to stay changed would be minimega.experiment_interface.

I would also prefer to have a way to set cluster.compute and cluster.control to localhost. Or have a way to set it automatically. Then, a fresh installation of FIREWHEEL could work without touching the config file at all. Not sure if minimega likes having the cluster as localhost though. If anybody has insight on that, it would be greatly appreciated.

@gregjacobus gregjacobus marked this pull request as draft December 23, 2025 16:56
@gregjacobus
Copy link
Collaborator Author

Okay, so apparently the log directories falling back to system.default_output_dir only happens when you run firewheel init. I personally never run this command. It feels like there's a better way to enforce the fallback, but maybe I just need to start running firewheel init more frequently.

Also, firewheel init would be a good place to set the cluster hostname if it isn't already set. That should be a fairly easy add.

Maybe the right way to do this is to wrap a call to firewheel init in firewheel start, possibly only if some config params are not set or something. Thoughts?

@mitchnegus
Copy link
Member

mitchnegus commented Dec 23, 2025

Okay, so apparently the log directories falling back to system.default_output_dir only happens when you run firewheel init. I personally never run this command. It feels like there's a better way to enforce the fallback, but maybe I just need to start running firewheel init more frequently.

Also, firewheel init would be a good place to set the cluster hostname if it isn't already set. That should be a fairly easy add.

Maybe the right way to do this is to wrap a call to firewheel init in firewheel start, possibly only if some config params are not set or something. Thoughts?

I would love it if we could bundle firewheel init into firewheel start. It seems very confusing to me that we have two commands that suggest "start FIREWHEEL" (even if there are actually differences, it's an implementation detail that users shouldn't bother with, especially if they would almost always run them both).

I'm a big fan of wrapping the functionality of firewheel init in firewheel start or just the firewheel command itself. For instance, this is a bit how config now works; the first time FIREWHEEL goes looking for the configuration (basically almost any firewheel command), it loads it from the template if it doesn't already exist.

Looks like firewheel init has mostly two pieces of functionality: (1) setting/loading config values, and (2) performing minimega setup. Feels like (1) should just get moved into the config functionality, and (2) should get moved into the start functionality, and we do away with firewheel init.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants