sim-cli: source simulation file from data directory #132

okjodom · 2023-10-06T23:15:16Z

Introduces SIM_LN_DATA_DIR from which sim-cli sources simulation files and writes simulation outputs.

carlaKC

Concept ACK, adding a data dir is super useful (esp in the docker context).

I do think we should think about the power of defaults / making the "getting started" journey as simple as possible with this change, specifically:

Assume sim.json to be the default simulation file unless otherwise specified.
Assume working directory to be the default data dir unless otherwise specified.

This makes for some quite nice options IMO:

sim-cli -> looks for simulation file in {pwd}/sim.json
sim-cli my-sim.json -> looks for simulation file at {pwd}/my-sim.json
sim-cli --data-dir=/Usr/simulations -> looks for simulation file at /Usr/simulations/sim.json
sim-cli my-sim.json --data-dir=/Usr/simulations -> looks for simulation file at /Usr/simulations/my-sim.json

I also really like having the selection option, which we can use to present other json files if we can't find the default value we were expecting.

sim-cli/src/main.rs

okjodom · 2023-11-06T00:12:43Z

Changes proposed introduce simln data dir, but I did not configure this to work with docker set up.
@Extheoisah, do you want to collab as part of #136

Extheoisah · 2023-11-06T11:07:05Z

Changes proposed introduce simln data dir, but I did not configure this to work with docker set up. @Extheoisah, do you want to collab as part of #136

Sure @okjodom
I'll also review the pr.

carlaKC

tACK, very nice improvement.

Just wondering about turning that arg into a flag, I can be convinced in either direction.

sim-cli/Cargo.toml

sim-lib/src/lib.rs

sim-cli/src/main.rs

carlaKC · 2023-11-15T14:21:35Z

@Extheoisah could you take a look to make sure this is going to work for docker-land and then we'll merge?

Also could use an update to the readme to explain these new snazzy features, I'm also happy to do that in a followup :)

sim-cli/src/main.rs

sim-lib/src/lib.rs

sim-cli/src/main.rs

okjodom · 2023-11-15T22:05:08Z

@sr-gi, in regards to your perspective against how this PR proposes to change how we source sim_file, I could consider a smaller proposal that only introduces data_dir for purposes of storing result outputs.

I'm excited about the new UX that always expects sim_file in data_dir and the helper ergonomics for picking one among many sim files, but I'll only push this if the UX is helpful and necessary.

Comments one, two and three are easy resolved if we can agree to change/preserve UX for passing sim_file to the cli

sr-gi · 2023-11-15T23:40:25Z

@sr-gi, in regards to your perspective against how this PR proposes to change how we source sim_file, I could consider a smaller proposal that only introduces data_dir for purposes of storing result outputs.

I'm excited about the new UX that always expects sim_file in data_dir and the helper ergonomics for picking one among many sim files, but I'll only push this if the UX is helpful and necessary.

Comments one, two and three are easy resolved if we can agree to change/preserve UX for passing sim_file to the cli

I'm open to being convinced if you and @carlaKC find this useful. It is just not the way I'd approach it

okjodom · 2023-11-16T01:02:45Z

I'm open to being convinced if you and @carlaKC find this useful. It is just not the way I'd approach it

Simple is always good. And what we had was simple + limited. I def polished the UX change too much (should have scoped it to just data dir addition) but don't want to take it back because I think users will like it :p

carlaKC · 2023-11-16T14:16:36Z

My 2c would be:

Remove the extension completion, since advanced users will know what it is.
Keep the tab completion and remember to user-test whether people respond to it the next time we do interviews.

okjodom · 2023-11-16T14:59:20Z

checked. trimmed out the extension completion

Extheoisah · 2023-11-16T15:46:51Z

@okjodom @carlaKC just tested the PR with docker and it's broken. I'm working on a PR to fix it. We can merge that after this pr is merged.

okjodom · 2023-11-16T16:08:15Z

@okjodom @carlaKC just tested the PR with docker and it's broken. I'm working on a PR to fix it. We can merge that after this pr is merged.

Thanks for the docker help. Lmk if you need to pair on it

Extheoisah · 2023-11-16T16:18:26Z

I've got it working already. One question though is if we want to have a data-dir for the docker setup? By default, the simulation CSV files are saved in the results folder in docker in the root. thoughts? cc @carlaKC

okjodom · 2023-11-16T17:32:16Z

I've got it working already. One question though is if we want to have a data-dir for the docker setup? By default, the simulation CSV files are saved in the results folder in docker in the root. thoughts? cc @carlaKC

Can we treat the data dir treated as a volume and have the csv files saved data_dir/results persistent beyond the containers runtime

Extheoisah · 2023-11-16T17:40:29Z

I've got it working already. One question though is if we want to have a data-dir for the docker setup? By default, the simulation CSV files are saved in the results folder in docker in the root. thoughts? cc @carlaKC

Can we treat the data dir treated as a volume and have the csv files saved data_dir/results persistent beyond the containers runtime

Yup, just made a pr that does that. I've also fixed the docker volume setup. It wasn't updated when we removed the node type from the sim.json config
pr here -> #159

sim-cli/src/main.rs

sim-lib/src/lib.rs

README.md

carlaKC

Code comments non-blocking. Would just like to clarify readme further then gg.

sim-cli/src/main.rs

README.md

carlaKC

🧡 thanks for updating!

sr-gi

Just one non-blocking nit.

LGTM

sim-cli/src/main.rs

Adds a simln data-dir cli param, but maintains the default value to cwd Sources the simulation file from this data dir Prompts the user to select a simulatin file if the default/configured file is not found

sr-gi

ACK 9975d8a

carlaKC reviewed Oct 17, 2023

View reviewed changes

sim-cli/src/main.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

okjodom force-pushed the config-dir branch from 56fb052 to 947944e Compare November 6, 2023 00:07

okjodom requested a review from Extheoisah November 6, 2023 00:13

okjodom marked this pull request as ready for review November 6, 2023 00:13

okjodom force-pushed the config-dir branch from 947944e to 7911df1 Compare November 8, 2023 15:39

okjodom requested a review from carlaKC November 8, 2023 15:39

carlaKC reviewed Nov 14, 2023

View reviewed changes

sim-cli/Cargo.toml Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

okjodom force-pushed the config-dir branch from 7911df1 to 6998634 Compare November 15, 2023 05:35

carlaKC approved these changes Nov 15, 2023

View reviewed changes

sr-gi reviewed Nov 15, 2023

View reviewed changes

sim-cli/src/main.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

okjodom force-pushed the config-dir branch from 6998634 to 24019ed Compare November 15, 2023 20:19

okjodom force-pushed the config-dir branch 2 times, most recently from a3a35ee to 628ec8e Compare November 16, 2023 00:52

okjodom requested a review from sr-gi November 16, 2023 00:52

okjodom force-pushed the config-dir branch from 628ec8e to d33272f Compare November 16, 2023 14:58

sr-gi reviewed Nov 16, 2023

View reviewed changes

okjodom force-pushed the config-dir branch from d33272f to 7c42807 Compare November 16, 2023 21:31

carlaKC mentioned this pull request Nov 17, 2023

fix: issue with docker config and volume setup #159

Merged

carlaKC reviewed Nov 17, 2023

View reviewed changes

sim-cli/src/main.rs Outdated Show resolved Hide resolved

sim-cli/src/main.rs Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Extheoisah added a commit to Extheoisah/sim-ln that referenced this pull request Nov 17, 2023

feat: update to path specification in bitcoin-dev-project#132

e01198b

okjodom force-pushed the config-dir branch from 7c42807 to e94eb20 Compare November 17, 2023 19:12

okjodom requested a review from carlaKC November 17, 2023 19:15

carlaKC approved these changes Nov 17, 2023

View reviewed changes

sr-gi approved these changes Nov 17, 2023

View reviewed changes

sim-cli/src/main.rs Outdated Show resolved Hide resolved

okjodom added 2 commits November 17, 2023 12:32

sim-cli: source simulation file from data directory

ef0a686

Adds a simln data-dir cli param, but maintains the default value to cwd Sources the simulation file from this data dir Prompts the user to select a simulatin file if the default/configured file is not found

docs: update readme with instructions on running the simulation

9975d8a

okjodom force-pushed the config-dir branch from e94eb20 to 9975d8a Compare November 17, 2023 20:32

sr-gi approved these changes Nov 17, 2023

View reviewed changes

Extheoisah added a commit to Extheoisah/sim-ln that referenced this pull request Nov 17, 2023

feat: update to path specification in bitcoin-dev-project#132

85082b3

carlaKC merged commit 6a056fe into bitcoin-dev-project:main Nov 20, 2023
2 checks passed

Extheoisah added a commit to Extheoisah/sim-ln that referenced this pull request Jan 25, 2024

feat: update to path specification in bitcoin-dev-project#132

a495e9d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sim-cli: source simulation file from data directory #132

sim-cli: source simulation file from data directory #132

okjodom commented Oct 6, 2023 •

edited

Loading

carlaKC left a comment

okjodom commented Nov 6, 2023

Extheoisah commented Nov 6, 2023

carlaKC left a comment

carlaKC commented Nov 15, 2023

okjodom commented Nov 15, 2023 •

edited

Loading

sr-gi commented Nov 15, 2023

okjodom commented Nov 16, 2023

carlaKC commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023 •

edited

Loading

carlaKC left a comment

carlaKC left a comment

sr-gi left a comment

sr-gi left a comment

sim-cli: source simulation file from data directory #132

sim-cli: source simulation file from data directory #132

Conversation

okjodom commented Oct 6, 2023 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

okjodom commented Nov 6, 2023

Extheoisah commented Nov 6, 2023

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC commented Nov 15, 2023

okjodom commented Nov 15, 2023 • edited Loading

sr-gi commented Nov 15, 2023

okjodom commented Nov 16, 2023

carlaKC commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023

okjodom commented Nov 16, 2023

Extheoisah commented Nov 16, 2023 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi left a comment

Choose a reason for hiding this comment

okjodom commented Oct 6, 2023 •

edited

Loading

okjodom commented Nov 15, 2023 •

edited

Loading

Extheoisah commented Nov 16, 2023 •

edited

Loading