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

A few minor tweaks from first use #7

Merged
merged 9 commits into from
Jul 17, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jul 8, 2023

I've opened this PR to just provide some tweaks I needed to get up and running.

@gpetretto
Copy link
Contributor

Hi, thanks for the updates. I have just one concern: unless this is properly documented and understood by the user, I am a bit afraid that that this could lead to unexpected submission to the wrong worker after a change in the yaml config file. I am wondering if it would be better to allow None for the worker just in the case only one worker is defined and still raise if more than one is available, in order to avoid any ambiguity. What do you think?

The description in the INSTALL.md is partially incorrect, since the Worker in jobflow-remote is quite different from a Fireworker in fireworks. We are also trying to avoid referencing Fireworks explicitly, since it will be replaced by a solution that will be better integrated with jobflow. Anyway, I can fix them when the PR is merged.

@ml-evs
Copy link
Member Author

ml-evs commented Jul 10, 2023

Hi, thanks for the updates. I have just one concern: unless this is properly documented and understood by the user, I am a bit afraid that that this could lead to unexpected submission to the wrong worker after a change in the yaml config file. I am wondering if it would be better to allow None for the worker just in the case only one worker is defined and still raise if more than one is available, in order to avoid any ambiguity. What do you think?

I think that is also sensible, yes (in fact that is how I initially coded it).

The description in the INSTALL.md is partially incorrect, since the Worker in jobflow-remote is quite different from a Fireworker in fireworks. We are also trying to avoid referencing Fireworks explicitly, since it will be replaced by a solution that will be better integrated with jobflow. Anyway, I can fix them when the PR is merged.

Indeed, I will be working with @davidwaroquiers this week to properly test this out with some other machines, and I will try to come up with some minimal documentation (now I know that lpad etc. is not required!) So hold-off any merging until then. (I was not able to make a draft PR, I think because this is a private repo in an organisation)

@gpetretto
Copy link
Contributor

Hi, thanks again for the updates and fixes. However, I would prefer to revert 4efbc52. The point of catching the exception there was to avoid showing the full traceback to the casual user, that will not want to dig into it. The cli_full_exc option allows you to see the full stack trace if needed. It can be enabled both with the jf -fe option or setting the jfremote_cli_full_exc variable in the setting if you always want the full stack trace. Would it be ok with you?

@ml-evs
Copy link
Member Author

ml-evs commented Jul 12, 2023

Hi, thanks again for the updates and fixes. However, I would prefer to revert 4efbc52. The point of catching the exception there was to avoid showing the full traceback to the casual user, that will not want to dig into it. The cli_full_exc option allows you to see the full stack trace if needed. It can be enabled both with the jf -fe option or setting the jfremote_cli_full_exc variable in the setting if you always want the full stack trace. Would it be ok with you?

Sure thing, these look like useful options!

@ml-evs
Copy link
Member Author

ml-evs commented Jul 12, 2023

I hadn't found -fe as it only appears in the help string for the base jf, not the subcommands. Perhaps it could be spread everywhere as a subcommand arg too?

@gpetretto
Copy link
Contributor

The advantage of having the global option is to avoid adding them everywhere when a new one is added and avoiding a lot of code duplication. Since the help is automatically generated by Typer there are limited options for what can be shown. I will try to see if there is a way to show them, but I would prefer not to transform the global options in local options for every command to keep it easier to maintain.

A compromise could be that the help of each command contains a message like "Run jf -h to see the global options" (not sure if there is an easy way to add it everywhere, or should be written explicitly for every command). Do you think this could have helped? Or it would have not been visible enough?

@ml-evs
Copy link
Member Author

ml-evs commented Jul 12, 2023

I think it would help new users, and could also be applied for flags like verbosity control. I'm not sure about Typer but click has a way of doing this with the root command: https://click.palletsprojects.com/en/8.1.x/complex/#the-root-command (I think) I can look into whether this is possible in Typer.

EDIT: from this discussion (tiangolo/typer#153) it looks like more effort than its worth 😅

@gpetretto
Copy link
Contributor

From a quick copy/paste of the click example it looks like that in click is the same (root options are not shown in the help of the subcommands), but maybe there is some option to tune. This may be outdated, but it seems that at the time there was no direct support for this in click: https://stackoverflow.com/questions/72897320/python-click-show-global-option-in-help-text-of-sub-command

An easy thing to do is:
Screenshot 2023-07-12 alle 16 43 09
What do you think?

@ml-evs
Copy link
Member Author

ml-evs commented Jul 12, 2023

What do you think?

Fine with me! I've removed the commits with the full traceback

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.

None yet

3 participants