RFC: remove `python robot.py xxx`

virtuald commented 6 months ago

In pursuit of making installation simpler, I'm contemplating changing the way that users launch their code (which has been the same since the 2015 season). It is currently a week before kickoff, but most of this is fairly simple.

For example, to run simulation:

Currently

py robot.py sim          # Windows
python robot.py sim      # Linux/macOS
./robot.py sim           # Linux/macOS w/executable bit set

And users must also add the following magic to their file:

if __name__ == "__main__":
    wpilib.run(MyRobot)

Proposed

Users no longer need to add the magic to their file. The run command will assume that the robot is in robot.py, and that it is a subclass of wpilib.RobotBase and it is defined in robot.py. If there are multiple robot classes, the user will need to add a setting to pyproject.toml.

robotpy sim              # Installed python script in path (windows or linux)
py -m robotpy sim        # Windows
python -m robotpy sim    # Linux/macOS

As an alternative (could do the above and this), a user could also create a file __main__.py

import robotpy.launcher

And then you could do:

py . sim          # Windows
python . sim      # Linux/macOS
./__main__.py sim # Linux/macOS w/executable bit set

As part of this, I propose removing the robotpy-installer command, and move all of its installation actions into the robotpy command via the extension mechanism that we currently use to register new subcommands of robot.py.

robotpy download
robotpy install
.. etc

In particular, this solves the chicken/egg problem I ran into when contemplating how to implement a sync command that a user could use to ensure their local robotpy installation is up to date. If a user has an import that isn't resolvable, then robot.py sync isn't possible to implement, so users would need to learn two commands.

Existing users have a lot of muscle memory that will need to be changed, but I think it would be better to do this change before the first official season instead of afterwards.

Not requiring robot.py to be ran directly will allow us to create better error messages for users with missing imports, make some aspects of unit tests simpler to change in the future, and provide a unified entrypoint for users to interact with robotpy.

Questions

Is this too risky to do a week before kickoff?
- Doing this isn't technically challenging, just have to shuffle a lot of pieces around
- ... haven't written any installation docs for frc-docs, so no risk that it doesn't get changed :-D
Do the advantages of a unified launcher outweigh destroying existing user's muscle memory?
- Existing documentation on the internet says do python robot.py XXX

ExploitSage commented 6 months ago

As someone with software engr experience in python, but no experience or personal stake in robotpy itself, while it is close to build season, this does look like a relatively low risk change and one that would bring a lot of quality of life improvement to the development pipeline and users. I also agree it would be better to "break" existing user muscle memory now rather than to do so later when you gain even more users through this season's official support.

As the only downside seems to be "annoyed existing users", and a last minute change while the upsides are good both for current and future users as well as for the project itself looking forward. Definitely a +1 from me.

sciencewhiz commented 6 months ago

I did a quick search through public repositories, and most that had files not named robot.py seemed to be test programs. But there do seem to be a lot of them/

For example, this one had both robot.py and mecanum.py, where robot.py is the main code, and mecanum.py ran the drive in test mode. https://github.com/Brown-County-FIRST-Robotics/1716-2022-robot

This team used version numbers in the filename for version control. https://github.com/Suave101/Jack_Sparrow/tree/2d1c2600ca0d688db742238b1d9476f8559479ab/Old%20Versions

lospugs commented 6 months ago

We are a new team switching to Python this year, so this won't affect us either way other than some updates to some training material that exists.

I think those that have used Python in the past without official support probably have the mentorship and skills to navigate this change very easily.

I also think this will greatly simplify things for new teams or teams with lighter mentor support.

auscompgeek commented 6 months ago

My 2c: the existing deploy command requiring robot code imports to work is a useful safety net against deploying code that doesn't compile. It's not perfect, but it's nice that inherent check is still there when the tests are skipped.

martinrioux commented 6 months ago

I like the proposal. I think that it is also doable to have the console print the new method should the scrip be ran directly.

Also, you could still run multiple robot code in the same folder using robotpy xx.py.

ArchdukeTim commented 6 months ago

Are the two methods incompatible? Under the hood the robotpy command would just call wpilib.run(<Something>), right?

virtuald commented 6 months ago

Are the two methods incompatible? Under the hood the robotpy command would just call wpilib.run(), right?

Technically, you could keep it, but then you would run into the chicken/egg problem mentioned above if someone tried to use the sync command.

If I were to make this change, I would move the argument processing code currently handled by wpilib.run into the robotpy-meta package. I would change wpilib.run to be a stub that tells you to run the other command.

gerth2 commented 6 months ago

I like using a robotpy command. I think the main mechanism, while nifty, is worse to teach from a pedagogy standpoint.

Breaking existing users is annoying, but I think they could get over it.

As the project expands, good development techniques and serving the masses (which should generally mean robot code starts in robot.py) is the right priority.

virtuald commented 6 months ago

As I haven't really heard much negative feedback on this, I'm going to go ahead with it locally and see what unforeseen chicken/egg problems I run into.

virtuald commented 6 months ago

Implemented at https://github.com/robotpy/robotpy-meta/pull/17

auscompgeek commented 6 months ago

It occurs to me that doing this will effectively make the robotpy package mandatory. Currently users can install the individual dependencies separately without using the robotpy package.

Not to say this is a bad thing, but noteworthy.

virtuald commented 6 months ago

It occurs to me that doing this will effectively make the robotpy package mandatory. Currently users can install the individual dependencies separately without using the robotpy package.

Yes.

This is actually a Good Thing because we currently pin dependency versions for core packages, and the version of the robotpy package is shown on the DS screen under versions, so it guarantees a particular set of packages are installed.

Additionally, the new sync functionality will force users to install a particular version of the robotpy package as well.

robotpy / mostrobotpy