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

Alpaca multi leg options and tweaks #702

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

GrizzlyEnglish
Copy link
Contributor

@GrizzlyEnglish GrizzlyEnglish commented Feb 13, 2025

Description by Korbit AI

What change is being made?

Enhance the Alpaca broker integration by adding support for multi-leg options orders, refining options symbol parsing, and updating order management logic.

Why are these changes being made?

These changes aim to expand the trading capabilities by including complex options strategies, address issues with options symbol handling, and improve the order submission process to align with current Alpaca functionalities. By introducing multi-leg order support and updating the parser for options symbols, the PR ensures better handling of options-related transactions and aligns with the latest Alpaca API updates.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

Copy link
Contributor

korbit-ai bot commented Feb 13, 2025

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

Copy link
Collaborator

@davidlatte davidlatte left a comment

Choose a reason for hiding this comment

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

A few things need to be updated to use existing functions for handling Options Symbols. Also, "legs" don't seem to be working correctly.

else:
# Submit each order
for order in orders:
self._submit_order(order)
Copy link
Collaborator

Choose a reason for hiding this comment

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

_submit_order() returns an order object with the Identifier filled in. This is "usually" the same as the order object passed in, but it is an important thing to track so that the proper items get passed to lifecycle methods like "on_new_order()" or "on_filled_order()" later. Please collect all of the orders passed back by _submit_order() and return them as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got this as well, I take the newly submitted orders and return those, rather than the passed in orders

@@ -425,6 +599,7 @@ def _submit_order(self, order):
"stop_price": str(order.stop_price) if order.stop_price else None,
"trail_price": str(order.trail_price) if order.trail_price else None,
"trail_percent": order.trail_percent,
"legs": order.legs
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only place that I can see where "legs" was ever set was in the other submit function "_submit_multileg_order()". It seems like this will never be valid at this point in time and that child_orders should be referenced instead to build out the required legs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented on the other, but the same thing applies here. As I understand alpacapy wants the legs in the order submission, not as an array of orders. I could be wrong though

Copy link
Collaborator

Choose a reason for hiding this comment

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

All of that is fine, you do have to Send it to Alpaca in the legs format it expects. However, you don't need to Store it in the order as "order.legs". This is because the structure of "legs" is different for each broker and it can be confusing months from now if there are updates that need to be made. I just think that "legs" should just live inside the alpaca.py file and not be a generic item in Order. Whenever a legs item is needed, it can just be created on the fly from order.child_orders (and each other broker can do the same thing for their own legs format)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I am with you almost got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok should all be fixed. Added tests for spread and condor

image

Moved to child ordres. Let me know what you think. I am opening it for real I think its good.

@GrizzlyEnglish GrizzlyEnglish marked this pull request as ready for review February 24, 2025 16:43
Copy link
Contributor

korbit-ai bot commented Feb 24, 2025

Important

Required App Permission Update

Noise Reduction Improvements

This update requests write permissions for Commit Statuses in order to send updates directly to your PRs without adding comments that spam notifications. Visit our changelog to learn more.

Click here to accept the updated permissions

To accept the updated permissions, sufficient privileges are required

Copy link
Contributor

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Performance Sequential Order Submission ▹ view
Files scanned
File Path Reviewed
lumibot/data_sources/alpaca_data.py
lumibot/brokers/alpaca.py
lumibot/entities/order.py

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings
Setting Value
Review Schedule Automatic excluding drafts
Max Issue Count 10
Automatic PR Descriptions
Issue Categories
Category Enabled
Documentation
Logging
Error Handling
Readability
Design
Performance
Security
Functionality

Feedback and Support

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

Comment on lines +568 to +570
for order in orders:
submittted_order = self._submit_order(order)
submittted_orders.append(submittted_order)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sequential Order Submission category Performance

Tell me more
What is the issue?

Orders are submitted sequentially in a loop, which could be slow for large batches of orders.

Why this matters

Sequential order submission increases total execution time and could cause delays in high-frequency trading scenarios.

Suggested change ∙ Feature Preview

Implement concurrent order submission using asyncio:

async def _submit_orders_concurrent(self, orders):
    tasks = [self._submit_order(order) for order in orders]
    return await asyncio.gather(*tasks)

Report a problem with this comment

💬 Chat with Korbit by mentioning @korbit-ai.

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